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.

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

Scale homepage carousel controls down for mobile #715

Merged
merged 5 commits into from Dec 17, 2018

Conversation

acdha
Copy link
Member

@acdha acdha commented Dec 13, 2018

Two of the titles added in #680 were wide enough that the homepage
carousel controls could overlap the text. This commit changes the sizing
rules for screens below the small breakpoint to keep the arrows centered
over the image rather than the combined image + overlay.

Before

“By the People” is fine on most devices:
simulator screen shot - iphone xr - 2018-12-13 at 10 35 30

… but “Letters to Lincoln” is not:

simulator screen shot - iphone xr - 2018-12-13 at 10 35 32

After

simulator screen shot - iphone xr - 2018-12-13 at 11 12 16

Two of the titles added in #680 were wide enough that the homepage
carousel controls could overlap the text. This commit changes the sizing
rules for screens below the small breakpoint to keep the arrows centered
over the image rather than the combined image + overlay.
@coveralls
Copy link

coveralls commented Dec 13, 2018

Coverage Status

Coverage remained the same at 70.22% when pulling a2c4748 on update-homepage-carousel-mobile-display into c5ec4ba on master.

@jbresner
Copy link
Collaborator

jbresner commented Dec 13, 2018

@acdha - here's a quick mockup for the iPhone (375px) with some of the specs I used. Also, change the title text from uppercase to title case (that should save some space as well).

crowd-home-375px

@jbresner
Copy link
Collaborator

At the smaller breakpoints, I'd recommend keeping the arrows at a fixed distance from the image and NOT centering it horizontally with all the text (Title, Description, CTA button) because as those vary in overall height, the left/right arrows end up bouncing up and down sometimes, which would be less than desirable for a user navigating.

@acdha
Copy link
Member Author

acdha commented Dec 13, 2018

Currently it's not aligned at all with the text — it's centered vertically over the image. I should be able to change that to stay consistently below the image but that's complicated by the viewport-specific sizing.

This avoids the indicators overlapping the overlay 
for display widths between 760px and 840px.
This changes the arrows to be just below the image
rather than centered over it. This required 
adjusting the maximum width of the title element
to maintain spacing between them.
@acdha
Copy link
Member Author

acdha commented Dec 13, 2018

@jbresner Here are some screenshots at various sizes from a Samsung S9 screen through a tablet:

screen shot 2018-12-13 at 13 16 20
screen shot 2018-12-13 at 13 16 27
screen shot 2018-12-13 at 13 16 31
screen shot 2018-12-13 at 13 16 35
screen shot 2018-12-13 at 13 18 18
screen shot 2018-12-13 at 13 18 29

@jbresner
Copy link
Collaborator

jbresner commented Dec 13, 2018

@acdha How much effort does it take to create a media query for the left/right arrows so the mobile versions (where the arrows aren't over the image) are solid black instead of white with outlines? I know it's not an ideal to have the two sets of arrows but because they're moving from images to a known solid color background, I'd rather they be as noticeable as possible in both cases.

@acdha
Copy link
Member Author

acdha commented Dec 13, 2018

We already have one so it's really just a question of the size of the extra SVG image(s)

@elainekamlley
Copy link
Collaborator

@acdha the last item you need is the smaller svg arrow correct?

@acdha
Copy link
Member Author

acdha commented Dec 14, 2018

The images scale down already (they're SVG so this looks fine) so it's just a question of whether we want separate images to flip the color or do something like put a CSS filter: invert(1) at the small breakpoint (IE11 doesn't support it but no phones run it either).

In this case we know the arrows will be on a solid white background so
we can simply invert them to reliably have black on white.
@acdha
Copy link
Member Author

acdha commented Dec 14, 2018

With a2c4748 it looks like this on mobile:

simulator screen shot - iphone xr - 2018-12-14 at 12 54 55
simulator screen shot - iphone xr - 2018-12-14 at 12 55 01

Copy link
Collaborator

@elainekamlley elainekamlley left a comment

Choose a reason for hiding this comment

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

LGTM

@rstorey rstorey merged commit b36fc6c into master Dec 17, 2018
@acdha acdha deleted the update-homepage-carousel-mobile-display branch December 17, 2018 19:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants