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.

Sign up for GitHub

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

Jump to bottom

Improve the CSS of the admin locale selection and login nav #5113

Merged
merged 1 commit into from
Jun 1, 2023
Merged

Improve the CSS of the admin locale selection and login nav #5113

merged 1 commit into from
Jun 1, 2023

Conversation

elia
Copy link
Member

@elia elia commented Jun 1, 2023 •
edited
Loading

Summary

Extracted from the Dark mode PR as these are overdue fixes that can be treated independently.

  • Make it reusable and move the icon from the CSS to the HTML
  • Add an icon to the expanded menu version
  • Use label to make the icon clickable
  • Add a title to the icon for improved usability
  • Align login icons and text

Before

image image

After

image image

Checklist

Check out our PR guidelines for more details.

The following are mandatory for all PRs:

The following are not always needed:

  • 📖 I have updated the README to account for my changes.
  • 📑 I have documented new code with YARD.
  • 🛣️ I have opened a PR to update the guides.
  • ✅ I have added automated tests to cover my changes.
  • 📸 I have attached screenshots to demo visual changes.

@github-actions github-actions bot added the changelog:solidus_backend Changes to the solidus_backend gem label Jun 1, 2023
@elia elia self-assigned this Jun 1, 2023
@elia elia force-pushed the elia/locale-selection-ui branch from 19873db to 8c6c07d Compare June 1, 2023 11:40
@elia elia changed the title Generalize the CSS style of the locale selection Improve the CSS of the admin locale selection and admin nav Jun 1, 2023
@elia elia changed the title Improve the CSS of the admin locale selection and admin nav Improve the CSS of the admin locale selection and login nav Jun 1, 2023
@elia elia marked this pull request as ready for review June 1, 2023 11:44
@elia elia requested a review from a team as a code owner June 1, 2023 11:44
@elia elia mentioned this pull request Jun 1, 2023
Merged
3 tasks
@codecov
Copy link

codecov bot commented Jun 1, 2023 •
edited
Loading

Codecov Report

Merging #5113 (3da9d65) into main (aa44e71) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #5113   +/-   ##
=======================================
  Coverage   88.64%   88.64%           
=======================================
  Files         563      563           
  Lines       13883    13883           
=======================================
  Hits        12306    12306           
  Misses       1577     1577

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

kennyadsl
kennyadsl reviewed Jun 1, 2023
View reviewed changes
Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

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

Thanks Eli, left a question before approving.

backend/app/views/spree/admin/shared/_locale_selection.html.erb
@@ -1,6 +1,7 @@
<% available_locales = Spree.i18n_available_locales %>
<% if available_locales.size > 1 %>
<div class="admin-locale-selection">
Copy link
Member

Choose a reason for hiding this comment

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

Is there a way to keep the old element along with the new (thinking about making the old wrapped into the new) so that Deface changes targeting this element will keep working?

Copy link
Member Author

Choose a reason for hiding this comment

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

@kennyadsl we can keep the old class name around, I don't think it's worth keeping the same tag name and using weird workarounds to get the same label-click effect. Would that be enough for you?

Copy link
Member

Choose a reason for hiding this comment

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

Let's try to find the best compromise; the class name already covers some usage of deface, if there are other changes to do on the host application side, I think it's acceptable, considering that it's on the Admin side and this is not the most used feature in the day to day of an operator.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

@mfrecchiami
Copy link
Contributor

Thanks @elia ❤️ 💯

@elia elia requested a review from kennyadsl June 1, 2023 12:40
@elia
Generalize the CSS style of the locale selection
3da9d65 
- Make it reusable and move the icon from the CSS to the HTML
- Add an icon to the expanded menu version
- Use label to make the icon clickable
- Add a title to the icon for improved usability
- Align login icons and text
- Keep the legacy class name to support existing customizations
@elia elia force-pushed the elia/locale-selection-ui branch from 8c6c07d to 3da9d65 Compare June 1, 2023 13:29
waiting-for-dev
waiting-for-dev approved these changes Jun 1, 2023
View reviewed changes
Copy link
Contributor

@waiting-for-dev waiting-for-dev left a comment

Choose a reason for hiding this comment

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

🚀

@elia elia merged commit 9b11dc7 into main Jun 1, 2023
11 checks passed
@elia elia deleted the elia/locale-selection-ui branch June 1, 2023 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@waiting-for-dev waiting-for-dev waiting-for-dev approved these changes

@kennyadsl kennyadsl kennyadsl approved these changes

@rainerdema rainerdema Awaiting requested review from rainerdema

Assignees

@elia elia

Labels
changelog:solidus_backend Changes to the solidus_backend gem
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@elia @mfrecchiami @waiting-for-dev @kennyadsl