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

Make gallery expanded view description scrollable #3624

Merged
merged 23 commits into from
Sep 2, 2024

Conversation

jsomeara
Copy link
Collaborator

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
  • I've written a descriptive PR title.
  • I've added/updated comments for large or confusing blocks of code.
  • I've included before/after screenshots above.

@jsomeara jsomeara self-assigned this Aug 22, 2024
@jsomeara
Copy link
Collaborator Author

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
Copy link
Member

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
Copy link
Collaborator Author

jsomeara commented Aug 23, 2024

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
Copy link
Member

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
Copy link
Collaborator Author

jsomeara commented Aug 23, 2024

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
Copy link
Collaborator Author

jsomeara commented Aug 24, 2024

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
Copy link
Member

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
Copy link
Collaborator Author

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
Copy link
Member

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
Copy link
Member

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
Copy link
Collaborator Author

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
Copy link
Member

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
Copy link
Collaborator Author

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

@jsomeara
Copy link
Collaborator Author

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
Copy link
Member

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
Copy link
Collaborator Author

jsomeara commented Aug 31, 2024

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
Copy link
Collaborator Author

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

@jsomeara
Copy link
Collaborator Author

jsomeara commented Sep 1, 2024

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.

Copy link
Member

@misaugstad misaugstad left a comment

Choose a reason for hiding this comment

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

Looks good, thank you!! I made just one change! The thumbs up/down icons were no longer inline with the severity buttons, which looked kinda funky. I reduced the padding so now they should be inline and I think it looks nicer!
Before you PR
Screenshot from 2024-09-02 12-01-01

Your code
Screenshot from 2024-09-02 11-48-40

After removing the padding
Screenshot from 2024-09-02 11-48-13

@misaugstad misaugstad merged commit 6699af4 into develop Sep 2, 2024
@jsomeara jsomeara deleted the 3622-gallery-expanded-view-scrollable-description branch September 2, 2024 19:02
@misaugstad misaugstad mentioned this pull request Sep 3, 2024
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.

Gallery expanded view description area needs to be scrollable if text doesn't fit
2 participants