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

Lf 4629 [nice to have] implement map footer redesign #3660

Merged

Conversation

Duncan-Brain
Copy link
Collaborator

@Duncan-Brain Duncan-Brain commented Jan 22, 2025

Description

Updates the footer styles + icons for map.

Notes:

  • Added a new button theme styles type but it feels like Button belongs in core components not Form. And maybe there is a better name than 'location-menu' taken from figma.
  • Considered adding "selected" option to match "active" state on button instead of custom styles.

Jira link: LF-4629

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Passes test case
  • UI components visually reviewed on desktop view
  • UI components visually reviewed on mobile view
  • Other (please explain)

Checklist:

  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • The precommit and linting ran successfully
  • I have added or updated language tags for text that's part of the UI
  • I have added "MISSING" for all new language tags to languages I don't speak
  • I have added the GNU General Public License to all new files

@Duncan-Brain Duncan-Brain requested review from a team as code owners January 22, 2025 01:54
@Duncan-Brain Duncan-Brain requested review from antsgar and SayakaOno and removed request for a team January 22, 2025 01:54
@Duncan-Brain Duncan-Brain marked this pull request as draft January 22, 2025 01:55
@Duncan-Brain Duncan-Brain marked this pull request as ready for review January 23, 2025 01:51
@Duncan-Brain Duncan-Brain changed the title Lf 4629 nice to have implement map footer redesign Lf 4629 [nice to have] implement map footer redesign Jan 24, 2025
Copy link
Collaborator

@SayakaOno SayakaOno left a comment

Choose a reason for hiding this comment

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

I haven't reviewed all files yet, but we need to display the filter and download buttons for non-admins 😄

Screenshot 2025-01-24 at 10 54 18 AM

@Duncan-Brain
Copy link
Collaborator Author

Thanks @SayakaOno I misread that conditional render! That makes a lot of sense 😂

@Duncan-Brain
Copy link
Collaborator Author

Duncan-Brain commented Jan 24, 2025

Duncans attention to detail strikes again -- should be fixed! ... but it looks like pnpm or actions is acting up again.

Copy link
Collaborator

@SayakaOno SayakaOno left a comment

Choose a reason for hiding this comment

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

I left comments, but it looks great!
I think the button height should be 64px, and we may need custom styling for the mobile view...?
Screenshot 2025-01-24 at 12 41 16 PM

@Duncan-Brain
Copy link
Collaborator Author

@SayakaOno thanks for review!

I am hesitant to set the height to 64px and since the width is normally fixed and would want to show all words if possible.

I chose to flex on height for label text because other languages might be much longer and I wanted to avoid truncating unless absolutely necessary. I added those lines for the long single word case.
Screenshot 2025-01-24 at 8 56 31 PM

Flex button item width with fixed menu width was also considered but if one of the text is much longer than the others it hogs the menu and squishes the others.

If you have any advice for me about this case, or how I would use the 64px height I would definitely appreciate it!

@antsgar
Copy link
Collaborator

antsgar commented Jan 28, 2025

I don't think the button needs to have a fixed 64px height per se but it's the vertical margins that are off -- they are 4px currently and should be 10px per the Figma designs. When those margins are updated, the button height ends up being pretty close to 64px in English where there's no text wrapping.

Copy link
Collaborator

@antsgar antsgar left a comment

Choose a reason for hiding this comment

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

Added a couple of comments but it looks great ❤️

packages/webapp/src/components/Form/Button/index.tsx Outdated Show resolved Hide resolved
@Duncan-Brain
Copy link
Collaborator Author

So we discussed with Loic today to remove the label from the mobile view and to truncate with an ellipsis if the text is too long -- no wrapping.

So that is updated. I bumped up the vertical padding to 8px (Loics magic padding margin number) curious though how you got 10px, clicking on the icon and text showed no margin/padding and this is what I was seeing for both:
Screenshot 2025-01-27 at 7 49 29 PM

@antsgar
Copy link
Collaborator

antsgar commented Jan 28, 2025

@Duncan-Brain re how I got the 10 px, I don't know if I ever shared this, but on Figma if you click on an element and press the Option key and then hover over other elements, you get the distance from that element to anything else. I can't take a screenshot because I don't know how to take on while keeping the effect 😂 but here's the tutorial from FIgma https://help.figma.com/hc/en-us/articles/360039956974-Measure-distances-between-layers

@Duncan-Brain
Copy link
Collaborator Author

@antsgar thanks for the link will remember to look for missing spacing next time.

@SayakaOno I see the 64px on the individual button you were talking about too now! I deleted it originally... and it would have maybe avoided the spacing issue Anto also noticed. Wondering if you were saying there are missing css properties that are on figma?

Screenshot 2025-01-28 at 9 36 01 AM

I typically try to remove as much Figma css as possible because it can be from parent or globals or it is not following html spec (notice the gap: -4px which is not possible) . So I try to just use it as a guideline.

Copy link
Collaborator

@SayakaOno SayakaOno left a comment

Choose a reason for hiding this comment

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

Looks great!

Wondering if you were saying there are missing css properties that are on figma?

No, I wasn't. I agree with your approach, we should have as little CSS as possible! (Edit: I’m not sure if that’s always true, but it’s my preference 😂)

Comment on lines +89 to +91
overflow: hidden;
text-overflow: ellipsis;
white-space: nowrap;
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can use @mixin truncateText() next time!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh thanks! I didn't know about that one! thanks 🙏

@Duncan-Brain Duncan-Brain added this pull request to the merge queue Jan 29, 2025
Merged via the queue into integration with commit 9fccd65 Jan 29, 2025
5 checks passed
@Duncan-Brain Duncan-Brain deleted the LF-4629-nice-to-have-implement-map-footer-redesign branch January 29, 2025 00:07
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.

3 participants