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

report(redesign): two rows for filmstrip on mobile #8563

Merged
merged 4 commits into from
May 23, 2019

Conversation

connorjclark
Copy link
Collaborator

@connorjclark
Copy link
Collaborator Author

Should I consume more of that whitespace? (remove most of the space between)

@paulirish
Copy link
Member

Can you describe what this is fixing?

This is the before/after i'm seeing:
image

@connorjclark
Copy link
Collaborator Author

@yuinchien how should the filmstrip images be laid out in mobile?

@yuinchien
Copy link
Collaborator

@hoten on mobile if we want to keep all 10 images, can we do 5 per row like in the image below?

Screen Shot 2019-04-25 at 10 58 24 AM

@connorjclark
Copy link
Collaborator Author

done
image

@connorjclark connorjclark changed the title Report ui filmstrip mobile report(redesign): two rows for filmstrip on mobile Apr 25, 2019
Copy link
Member

@exterkamp exterkamp left a comment

Choose a reason for hiding this comment

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

This LGTM, very nice change. But, a possible nit: I feel like I'd maybe like it better to always be the proper spacing between them and center the whole 5x2 block. Dunno, might look weird that it doesn't use the whole space, but I feel like when it is stretched really far it also looks weird. Feel free to ignore this tho 😄

Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

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

LGTM!

I kinda agree with @exterkamp though that the spacing looks a little weird when they get too far apart.

Just throwing this out there as a potential option, maybe we could show 1 row of just however many there are space for? All CSS impl might be a bit messy, but it might feel more natural to keep the spacing even and reveal more images as room allows

@connorjclark
Copy link
Collaborator Author

Just throwing this out there as a potential option, maybe we could show 1 row of just however many there are space for? All CSS impl might be a bit messy, but it might feel more natural to keep the spacing even and reveal more images as room allows

So if there is room for 8 images, the last two won't be displayed?

@patrickhulce
Copy link
Collaborator

So if there is room for 8 images, the last two won't be displayed?

Well I don't think we would want to just hide the last ones, more like the intermittent ones. i.e. if there's space for 8 we hide frame 3 and 7 or something

@connorjclark
Copy link
Collaborator Author

connorjclark commented Apr 26, 2019

I don't think we should elide random frames. And the CSS that would come with it is weird I bet. @yuinchien / @hwikyounglee any ideas to deal with the spacing issues here?

@yuinchien
Copy link
Collaborator

How about for mobile & desktop, we scale the image and keep the gutter size constant?
Mobile will be 5 images per row, gutter size 10px, 2 rows total.
Desktop 10 images per row, gutter size 20px, 1 row total.

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

this seems like a fine solution to me that improves the status quo with basically no downside, it just may not be optimal. If others feel strongly about the extra spacing issue raised in discussion I suggest:

  • a firm idea that we can actually move on, or
  • landing this and opening an issue for discussion

Yeah?

@connorjclark
Copy link
Collaborator Author

👍 to landing

@brendankenny
Copy link
Member

we had LGTMs with "a possible nit" and "looks a little weird when they get too far apart", so let's call them LGTMs with the desire for tweaks in the future if folks want to drive them :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants