Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Amend margins on mobile displays #662

Closed
wants to merge 70 commits into from
Closed

Amend margins on mobile displays #662

wants to merge 70 commits into from

Conversation

marek-lach
Copy link
Member

@marek-lach marek-lach commented Aug 29, 2019 •
edited
Loading

Fixes #655 and also most of #604.

Edit: Better addressed by #663. Closing this issue now.

@marek-lach marek-lach changed the title Update _global.scss Amend margins a little, to not be too close to the screen edge on high resolution displays and mobile Aug 29, 2019
@codecov
Copy link

codecov bot commented Aug 29, 2019

Codecov Report

Merging #662 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #662   +/-   ##
=======================================
  Coverage   34.58%   34.58%           
=======================================
  Files          68       68           
  Lines        8020     8020           
  Branches     1890     1890           
=======================================
  Hits         2774     2774           
  Misses       4468     4468           
  Partials      778      778

1 similar comment
@codecov
Copy link

codecov bot commented Aug 29, 2019 •
edited
Loading

Codecov Report

Merging #662 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #662   +/-   ##
=======================================
  Coverage   34.58%   34.58%           
=======================================
  Files          68       68           
  Lines        8020     8020           
  Branches     1890     1890           
=======================================
  Hits         2774     2774           
  Misses       4468     4468           
  Partials      778      778

@elegaanz
Copy link
Member

Looks like it is too much now…

image

image

@elegaanz
Copy link
Member

Still the same for me… 😕 I can try to have a look tomorrow or on Saturday if you want.

@marek-lach
Copy link
Member Author

I'll see what I can do...

@marek-lach marek-lach changed the title Amend margins a little, to not be too close to the screen edge on high resolution displays and mobile Amend margins, to not be too close to the screen edge on high resolution displays and mobile Aug 30, 2019
@marek-lach
Copy link
Member Author

@AnaGelez All tested and seems to work well on my end.

@marek-lach marek-lach added Rendering How elements're rendered out for the end user S: Ready for review This PR is ready to be reviewed labels Aug 30, 2019
@marek-lach marek-lach mentioned this pull request Aug 30, 2019
Closed
@elegaanz
Copy link
Member

The article content is indeed better (maybe even a bit too much), but some elements that shouldn't have margins now have (article cover/heading, and all the info at the bottom from the license to the comment area). 😕

@marek-lach marek-lach removed the S: Ready for review This PR is ready to be reviewed label Aug 30, 2019
@marek-lach marek-lach added the S: Ready for review This PR is ready to be reviewed label Sep 1, 2019
@marek-lach marek-lach changed the title Amend margins Amend margins on mobile displays, and fix content overflow Sep 1, 2019
@marek-lach marek-lach mentioned this pull request Sep 1, 2019
Closed
@marek-lach marek-lach changed the title Amend margins on mobile displays, and fix content overflow Amend margins on mobile displays, and fix area content overflow Sep 1, 2019
@marek-lach marek-lach changed the title Amend margins on mobile displays, and fix area content overflow Amend margins on mobile displays, and fix content area overflow Sep 1, 2019
@marek-lach marek-lach changed the title Amend margins on mobile displays, and fix content area overflow Amend margins on mobile displays Sep 1, 2019
@marek-lach marek-lach requested a review from elegaanz September 1, 2019 16:12
elegaanz
elegaanz requested changes Sep 1, 2019
View reviewed changes
Copy link
Member

@elegaanz elegaanz left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry but I still have issues… I can go into details if you want, but I also made another PR that should fix these issues, because I felt you were struggling with CSS here… So you can either tell me if #663 works for you, or I can tell you what is still wrong with this one, and you cant try to fix it if you really want to do it yourself.

@marek-lach
Copy link
Member Author

I felt you were struggling with CSS here

You are right. It's a lot of guess work for me, I am not really natural at it :-)

@marek-lach marek-lach closed this Sep 1, 2019
@marek-lach marek-lach deleted the marek-lach-patch-1-2 branch September 1, 2019 18:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@elegaanz elegaanz elegaanz requested changes

Assignees
No one assigned
Labels
Mobile Issues affecting only mobile UX Rendering How elements're rendered out for the end user S: Ready for review This PR is ready to be reviewed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Margins are too narrow on mobile
2 participants
@marek-lach @elegaanz