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

chore: use Lucide icons instead of SVG backgrounds and simplify DOM #7101

Merged
merged 5 commits into from
Aug 26, 2024

Conversation

VIKTORVAV99
Copy link
Member

@VIKTORVAV99 VIKTORVAV99 commented Aug 22, 2024

Issue

We were using SVG background instead of Icon components.

Description

Removes the SVG backgrounds and replaces them with Lucide icons. It also further simplifies the DOM and styling of the Accordion component to fix a title overflow issue in some languages.

Double check

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

@VIKTORVAV99 VIKTORVAV99 changed the title chore: use Lucide icons instead of SVG b chore: use Lucide icons instead of SVG backgrounds and simplify DOM Aug 22, 2024
@VIKTORVAV99

This comment was marked as resolved.

@VIKTORVAV99
Copy link
Member Author

@Alportan this is the PR that uses the new Lucide icon you added, however the Figma seems to miss the replacement icon for the aggregated card (the one on top if you switch to daily, monthly or yearly) so I just picked one of the graph icons without the axis.

Could you double check this one?

@VIKTORVAV99 VIKTORVAV99 marked this pull request as ready for review August 26, 2024 08:08
@Alportan
Copy link
Contributor

Alportan commented Aug 26, 2024

@Alportan this is the PR that uses the new Lucide icon you added, however the Figma seems to miss the replacement icon for the aggregated card ... so I just picked one of the graph icons without the axis.

Good catch, I think the graph icon looks great! 🙌

Could you double check this one?

I have a few comments, that now, with the new icons, the icon gets really close to the card title. What's the current padding between the icon and the title? I realised I was the culprit adding it in Figma 🙈 Let's increase it midway to 6px.

image

Additionally, can we use the same icon as we do on the card on the Preliminary badge?

image image

@VIKTORVAV99
Copy link
Member Author

@Alportan this is the PR that uses the new Lucide icon you added, however the Figma seems to miss the replacement icon for the aggregated card ... so I just picked one of the graph icons without the axis.

Good catch, I think the graph icon looks great! 🙌

Could you double check this one?

I have a few comments, that now, with the new icons, the icon gets really close to the card title. What's the current padding between the icon and the title? I realised I was the culprit adding it in Figma 🙈 Let's increase it midway to 6px.

image Additionally, can we use the same icon as we do on the card on the `Preliminary` badge?

image image

As discussed in Slack I tweaked the style changes.

For fixing the preliminary I'll do that in a follow up PR (It's already in the works) to keep the scope of this PR mostly to the icons themselves and not so much the logic around them.

Copy link
Contributor

@Alportan Alportan 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! 🙌

@VIKTORVAV99 VIKTORVAV99 enabled auto-merge (squash) August 26, 2024 09:53
@VIKTORVAV99 VIKTORVAV99 merged commit 3cea711 into master Aug 26, 2024
21 checks passed
@VIKTORVAV99 VIKTORVAV99 deleted the vik/replace_svg_backgrounds_with_icons branch August 26, 2024 09:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file frontend 🎨
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants