fix: restore logo to footer #1761
Conversation
|
ETA for review: 14 June 2024 |
There was a problem hiding this comment.
Reviewed:
- Pulled down and visually tested on browser. HFLA logo was restored as shown in video.
- Great idea with adding the util functions. They significantly enhance readability by eliminating the clutter of multiple
entities, which can otherwise cause text to break awkwardly on narrow viewports like you mentioned.
As far as I know, we don't have a policy on responsiveness in design or dev yet, but I believe it's an important direction for us to consider. @ryanfchase might have more insights or corrections. However, the current PR's responsive behavior looks good to me.
|
@ryanfchase @aqandrew We should also consider if we want to apply this implementation to the rest of the codebase, by refactoring If we do want to, what would be the most efficient way to do it?
|
There was a problem hiding this comment.
I've never used gap and inline-margin before, those seem useful. I assume you could also just do margin-left, but inline-margin maybe implies that we want to add this buffer space between this element and the element "before it".
Or maybe it's not that complicated, approved!
There was a problem hiding this comment.
@aqandrew Great job on this issue, especially going above and beyond!
- PR done on correct branch
- PR linked to correct issue
- PR does more than just fix the issue, it goes a step further and considers responsiveness
- The Figma screenshot links to a blank page for me
- Logo is imported correctly from the existing assets folder
The only question I have is regarding the removal of the //TODO comment. Since it was asking about re-adding privacy policy statements, should that comment be added back in?
Otherwise, great job, approved!
@DrAcula27 Chimming in regarding the TODO comment, we are all good on removing it as those TODOs has been completed probably more than a year ago. That comment is very old 😃 |
I've added this to the agenda for next week. I guess if I personally had to pick, it would be reaaaaaaally boring to go through the project and convert all instances of it. Even as |
|
@aqandrew regarding your question...
Yes, just large screens at the moment. I actually am not seeing what problems we might run into for the new linebreaking behavior, could you point out an example? |
@DrAcula27 the link is indeed broken here, but it is working on the original issue #1750 ! |
Sorry, I was rushing and forgot what you wrote earlier. If the new behavior is what you outlined at the beginning of your PR description, then yes. This new behavior is acceptable, thank you! |
|
@ryanfchase Yes, gap and margin-inline are very useful. Margin-inline adds a margin on both the left and right sides, so that there's enough spacing on the logo's right side when the screen is small enough for the logo to press up next to the @DrAcula27 I removed that TODO because the footer elements currently do follow the order that Design specified in Figma, so it's already done. Thank you for confirming @Skydodle! Off-topic: refactoring non-breaking spaces in the rest of the codebase won't be a heavy lift. Once this PR is merged,
|
Fixes #1750
mainbranchModifies the linebreaking behavior of the footer text on smaller screens. The original footer text, with lots of
entities between each text item, would break to new lines whenever the viewport isn't wide enough (this occurred inLastUpdatedtoo). Also placement/amount of s was inconsistent, and they made the source code difficult to read. The new text layout:footerItems, which gets spaces replaced with non-breaking spaces in JSX so that footer text is easier to read when it needs to be updated|separators, and the logoIs it the case that we only support large screens? If so, is this new linebreaking behavior acceptable?
screenshot of Figma (from issue description)
Footer's responsive behavior after first commit
Screen.Recording.2024-06-11.at.4.20.51.PM.mov
Footer's responsive behavior after refactoring
Screen.Recording.2024-06-11.at.4.22.18.PM.mov