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

perf: use icons instead of SVGs in TimeSlider #6982

Merged
merged 12 commits into from
Aug 8, 2024

Conversation

VIKTORVAV99
Copy link
Member

@VIKTORVAV99 VIKTORVAV99 commented Jul 15, 2024

Description

Splits out icon changes in TimeSlider from #6911 so it is easier to review that PR and can be merged faster.

The reasoning for doing this and not using the SVG files as backgrounds are two fold. One we always need them so it's smart to bundle them directly with the code and reduce the http calls we make. Two they no longer match our visual design of the other icons and changing them to Font Awesome versions fixes that as we use Font Awesome in other parts of the app.

Preview

image image image image image image

Double check

  • I have run pnpx prettier@2 --write . and poetry run format in the top level directory to format my changes.

@VIKTORVAV99 VIKTORVAV99 requested review from a team and madsnedergaard and removed request for a team July 15, 2024 07:44
@VIKTORVAV99 VIKTORVAV99 changed the title use icons instead of SVGs in TimeSlider perf: use icons instead of SVGs in TimeSlider Jul 15, 2024
@VIKTORVAV99 VIKTORVAV99 mentioned this pull request Jul 16, 2024
1 task
Copy link
Member

@madsnedergaard madsnedergaard left a comment

Choose a reason for hiding this comment

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

How about keeping existing icons (but moving them to React components) for now, and then we later on take a thorough revisit to all icon usage instead? I personally don't think it's worth using a consistent icon package if it means sacrificing eligibility and UX
¯\_(ツ)_/¯

@VIKTORVAV99
Copy link
Member Author

VIKTORVAV99 commented Jul 16, 2024

How about keeping existing icons (but moving them to React components) for now, and then we later on take a thorough revisit to all icon usage instead? I personally don't think it's worth using a consistent icon package if it means sacrificing eligibility and UX

¯\_(ツ)_/¯

The problem with the existing icons are that they are not uniform and scaling them is not the easiest without a lot of manual work (or runtime calculations).

There are outline versions of these icons as well but I would much prefer to use the icon library than have them as custom components since that means we will use a consistent icon style. I'd be happy to discuss it on the design review it's Alex though if we have the time (tomorrow).

@madsnedergaard
Copy link
Member

How about keeping existing icons (but moving them to React components) for now, and then we later on take a thorough revisit to all icon usage instead? I personally don't think it's worth using a consistent icon package if it means sacrificing eligibility and UX
¯_(ツ)_/¯

The problem with the existing icons are that they are not uniform and scaling them is not the easiest without a lot of manual work (or runtime calculations).

There are outline versions of these icons as well but I would much prefer to use the icon library than have them as custom components since that means we will use a consistent icon style. I'd be happy to discuss it on the design review it's Alex though if we have the time (tomorrow).

Did you have a chance to discuss this yesterday? :)

@VIKTORVAV99
Copy link
Member Author

How about keeping existing icons (but moving them to React components) for now, and then we later on take a thorough revisit to all icon usage instead? I personally don't think it's worth using a consistent icon package if it means sacrificing eligibility and UX

¯_(ツ)_/¯

The problem with the existing icons are that they are not uniform and scaling them is not the easiest without a lot of manual work (or runtime calculations).

There are outline versions of these icons as well but I would much prefer to use the icon library than have them as custom components since that means we will use a consistent icon style. I'd be happy to discuss it on the design review it's Alex though if we have the time (tomorrow).

Did you have a chance to discuss this yesterday? :)

I did not, so let's pause this PR for a bit until next week.

I don't necessarily believe that this impacts the UX in a negative way though. Having a uniform icon style brings it's own UX benefits.

But I'll test it a bit on lower resolution devices as well and see how it looks.

@tonypls
Copy link
Collaborator

tonypls commented Aug 6, 2024

Code looks good to me, but I think it's more a UX change that is suited to @Alportan to review

@Alportan
Copy link
Contributor

Alportan commented Aug 6, 2024

Code looks good to me, but I think it's more a UX change that is suited to @Alportan to review

Just a heads up on the dark mode: the contrast could use a little boost. We might want to tweak the slider nob's background and make it darker, as the white icons are blending into the nob's grey.

I've put together a quick GIF using what I call the "blur of accessibility" to illustrate the eligibility and UX concern Mads mentioned. I use this whenever I'm in doubt of contrast, as it can highlight any contrast issues (either in color or shape, in this case filled icons vs outlined).

Screen Cast 2024-08-06 at 4 03 51 PM

You can see the outlined icons lose their contrast a bit. Here are my suggestions:

  • Bump the nob size to 28px
  • Bump up icon size to 20px
  • Darken the nob a bit
  • Add an outline to the nob for contrast
  • Change the icon when no zone is selected to chevrons-left-right

I've mocked up these changes in Figma. Feel free to play with the blur to see the difference. Let me know what you think @VIKTORVAV99 ! 🙌

image

@VIKTORVAV99
Copy link
Member Author

The proposed updates should be live now, let me know what you think 🙂

Copy link
Collaborator

@tonypls tonypls left a comment

Choose a reason for hiding this comment

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

LGTM

@VIKTORVAV99
Copy link
Member Author

VIKTORVAV99 commented Aug 8, 2024

@Alportan do you have any additional feedback or can I merge this?

@Alportan
Copy link
Contributor

Alportan commented Aug 8, 2024

@Alportan do you have any additional feedback or can I merge this?

Looking good, green light from my side! 🎉 You can merge! 😎

@VIKTORVAV99 VIKTORVAV99 enabled auto-merge (squash) August 8, 2024 09:24
@VIKTORVAV99 VIKTORVAV99 merged commit c8dce3e into master Aug 8, 2024
21 checks passed
@VIKTORVAV99 VIKTORVAV99 deleted the vik/time_slider_changes branch August 8, 2024 09:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants