ProjectSidewalk / SidewalkWebpage

Project Sidewalk web page
http://projectsidewalk.org
MIT License
84 stars 25 forks source link

Make gallery expanded view description scrollable #3624

Closed jsomeara closed 2 months ago

jsomeara commented 3 months ago

Resolves #3622

Previously, when the height was small enough, the description would overflow the expanded view modal. This is now fixed by setting the overflow property in the description body to auto and also some minor JS scripting to deal with the cases where css alone is not enough (in some situations where the height is very very small).

Before/After screenshots (if applicable)

image image

Testing instructions
  1. Go to the gallery
  2. Find an entry with an extremely long description. (Make one if you can't find something)
  3. Confirm that it's now scrollable
  4. You can also try resizing the window to see how it adapts.
Things to check before submitting the PR
jsomeara commented 3 months ago

Something I was wondering here: Should there still be some padding at the bottom? Right now, it's made so that padding isn't respected. Is this fine or should we do something else?

misaugstad commented 3 months ago

By padding, do you mean padding between the bottom of the text area and the bottom of the "expanded view" area? Or between the expanded view and the bottom of the page?

jsomeara commented 3 months ago

padding between the bottom of the text area and the bottom of the "expanded view" area

This one. Right now, if there's an overflow, it utilizes the expanded view area as much as possible. Anything beyond that will result in a scroll bar.

misaugstad commented 3 months ago

Thanks! I think that a bit of padding would be best! It looks a bit squished. And we don't get that much from it since you can't actually read that 4th line anyway!

jsomeara commented 3 months ago

Thanks! I think that a bit of padding would be best! It looks a bit squished. And we don't get that much from it since you can't actually read that 4th line anyway!

Agreed! I'll update my PR shortly.

jsomeara commented 3 months ago

Added 15px minimum of padding. Also, I reduced the height of the tags container purely for aesthetics (I know this wasn't originally part of this PR but I felt it was wasting a lot of space and could be reduced to avoid the description needing to scroll in the first place). Let me know if I should undo this height reduction.

Screenshot: image

The padding remains even when shrunk down to a very small height: image

misaugstad commented 3 months ago

I'm mostly really happy with it! Was able to break it with some stress testing by adding a bunch of tags and then a long description! Screenshot from 2024-08-27 11-32-42

Maybe this just means including a max-height for the tags as well. I'd like to at least feel very confident that we can fit two rows of tags without scrolling!

jsomeara commented 3 months ago

The issue you referenced above was caused by a race condition I didn't notice earlier. The margin calculation function ran before the modal/expanded view was fully rendered, causing inaccurate numbers. Should be fixed now! I also reduced the font size of the gallery tags and added a max height to the gallery tags container.

image

misaugstad commented 3 months ago

I also reduced the font size of the gallery tags and added a max height to the gallery tags container.

Okay! I'm cool with decreasing the font size a bit, though I'd rather do it for all the text in the expanded view, not just the tags. Especially because the tags might be the most important info here!

For Gallery at least, I'd also like to lean towards using vw whenever possible, since that's what we've been using for most of the page, and should help to keep the page looking more responsive to changes in screen size!

misaugstad commented 3 months ago

Maybe there's a way to do away with your JS code to calculate the margin/padding on the bottom by using vw and/or %? Sorry I haven't looked that carefully into what the JS does yet so no worries if I'm making assumptions that don't hold up!

jsomeara commented 3 months ago

Okay! I'm cool with decreasing the font size a bit, though I'd rather do it for all the text in the expanded view, not just the tags. Especially because the tags might be the most important info here!

Decreased expanded view info font size a bit and used vw in the process. If it's too small now, please don't hesitate to let me know!

Maybe there's a way to do away with your JS code to calculate the margin/padding on the bottom by using vw and/or %? Sorry I haven't looked that carefully into what the JS does yet so no worries if I'm making assumptions that don't hold up!

Absolutely, and your assumptions are 100% correct! The info section at the bottom that contains the severity, timestamp, tags, description actually had a version of this where it split each piece of information into sections. The description was 60% height and the tags were 40% height. The issue was that the same div also included the timestamp, resulting in a total height greater than 100%, leading to the overflow issues for the description. For some reason, I never noticed this layout design or issue when I first worked on the issue! I switched it to using CSS now instead of JS and much prefer the updated solution.

Screenshot: image

misaugstad commented 2 months ago

I'm happy with the new text size! Though I noticed a few visual issues. The first is that the bottom of the severity smileys are being cut off now (at least for me), and I'm not happy with the space between the Tags header and the list of tags when there's only one row. The list of tags look like they're closer to the Description header than the Tags header!

Before Screenshot from 2024-08-29 10-35-04

After Screenshot from 2024-08-29 10-35-13

And I'd really like to get the 2nd row of tags without showing the scroll! And it's totally fine for that space to come at the expense of space for the description, because tags are used wayyy more frequently than the description field. Screenshot from 2024-08-29 10-38-27

jsomeara commented 2 months ago

Definitely agreed. Those issues are deal breakers. Going to work on a fix today.

jsomeara commented 2 months ago

I was able to reproduce severity smileys being cut off on the Project Sidewalk Seattle production site too:

image

Not totally sure what's going on here. I've analyzed it a bit in dev tools and can't find any reason why it'd be doing that. It felt like it was happening randomly too. I would resize the browser window size by 1px and then the issue would go away. I thought it might be a browser-specific issue. To test this hypothesis, I downloaded Firefox but also saw that the issue occurred there too. I tried increasing the vertical size of the viewbox in the smiley SVGs and that seemed to fix the issue. But I'm very confused as to why that worked because everything should be fitting in the bounding box...? So strange! For this bug, should I create a separate issue or try to fix it in this GitHub issue?

I know you also pointed out the tags being distant from the tags header and also the tags not showing at least two rows. Hopefully these other visual issues are fixed now with my recent commits!

misaugstad commented 2 months ago

I was able to reproduce severity smileys being cut off on the Project Sidewalk Seattle production site too:

Oh no!

should I create a separate issue or try to fix it in this GitHub issue?

If it seems like a pretty easy fix, I think it's fine to just throw it in to this PR! Already doing some adjustments to the area so I think it's fine :) If it seems like it would be more involved, then we can make a new ticket for it

jsomeara commented 2 months ago

I fixed the issue by increasing the size of the icons' vertical viewbox very slightly. 150 -> 155. While I was fixing the issue, I caught a bug in my previous code and that's been fixed now too.

In my previous comment, I forgot to include a screenshot, so putting one here now:

image

I'm gonna make one more commit to lessen the distance between the tags header and tags and then this should be ready.

jsomeara commented 2 months ago

And the commit I referenced at the bottom of my last message is now finished!

jsomeara commented 2 months ago

While I was working on another issue, I noticed the severity icons can also be cut off horizontally too. I expanded the horizontal viewbox too. Hopefully everything should be good now! Ready for review.