Skip to content

Change language picker icon #4503

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

Merged
merged 25 commits into from
Mar 28, 2022
Merged

Change language picker icon #4503

merged 25 commits into from
Mar 28, 2022

Conversation

ArkinSolomon
Copy link
Contributor

@ArkinSolomon ArkinSolomon commented Mar 18, 2022

Fixes #3372. I know this had 2 pull requests created already, with the most recent one dying pretty quickly. It was a pretty simple change to use the new image. I did slightly modify the icon to be white only. It works fine for both light and dark modes.

Screen Shot 2022-03-18 at 1 50 32 PM

icon128px-exported-black

@Trott
Copy link
Member

Trott commented Mar 19, 2022

Where did the icon come from originally?

@ArkinSolomon
Copy link
Contributor Author

It came from http://www.languageicon.org/, see #3503.

@Trott
Copy link
Member

Trott commented Mar 22, 2022

@nodejs/website

@XhmikosR
Copy link
Contributor

It shouldn't be a PNG.

@XhmikosR XhmikosR closed this Mar 22, 2022
@XhmikosR
Copy link
Contributor

It shouldn't be a PNG.

@XhmikosR XhmikosR reopened this Mar 22, 2022
@XhmikosR XhmikosR requested review from XhmikosR and removed request for XhmikosR March 22, 2022 04:59
Copy link
Contributor

@XhmikosR XhmikosR left a comment

Choose a reason for hiding this comment

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

Please use SVG files after optimizing it and no redundant attributes.

@ArkinSolomon
Copy link
Contributor Author

I have changed the icons to an svg by converting it from .eps, and manually added light and dark variants and used css to change them when the theme changes.

@ArkinSolomon ArkinSolomon requested a review from XhmikosR March 23, 2022 06:30
Since dark mode transition time is used in multiple places, I added a variable so if it needs to be changed it can be changed easily.
@ArkinSolomon ArkinSolomon requested a review from XhmikosR March 23, 2022 16:31
@ArkinSolomon ArkinSolomon requested a review from XhmikosR March 23, 2022 16:42
@ArkinSolomon ArkinSolomon requested a review from osk2 March 24, 2022 06:10
Copy link
Contributor

@osk2 osk2 left a comment

Choose a reason for hiding this comment

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

LGTM

@ArkinSolomon
Copy link
Contributor Author

Is this PR good for merging @XhmikosR ?

@XhmikosR
Copy link
Contributor

Sorry, I can't review the result (this is one of the cases PR previews would go a long way).

If someone can confirm everything looks good on both themes, that's good enough from my side judging by the code changes.

@Trott
Copy link
Member

Trott commented Mar 28, 2022

Sorry, I can't review the result (this is one of the cases PR previews would go a long way).

@XhmikosR If you're online right now, here's a preview I'll keep around for the next 30 minutes or so: https://8080-arkinsolomon-nodejsorg-yges5368tg1.ws-us38.gitpod.io/

@XhmikosR
Copy link
Contributor

Thanks @Trott!

Confirmed LGTM. The same optimizations could be made to the dark switcher icons later (inline them, optimize them, toggle the d-none class etc)

@Trott Trott merged commit 7ee3902 into nodejs:main Mar 28, 2022
@ArkinSolomon ArkinSolomon deleted the change-lang-icon branch March 28, 2022 16:57
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.

Change language-picker icon so it's not the Google Translate icon
5 participants