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

confusing icons hover issue fix #2255 #2438

Merged
merged 8 commits into from
Jun 3, 2024

Conversation

luckyklyist
Copy link
Contributor

@luckyklyist luckyklyist commented Sep 14, 2023

Fix Confusing Icons #2255

Changes Made

I've addressed the issue of confusing icons that occurred due to the Sass hover styles. To enhance the user experience, I reorganized the code related to hover-specific styles. Now, users can clearly see whether they've added a sketch without needing to hover over the button.

Screen.Recording.2023-09-14.at.2.52.21.PM.mov

I have verified that this pull request:

  • has no linting errors (npm run lint)
  • has no test errors (npm run test)
  • is from a uniquely-named feature branch and is up to date with the develop branch.
  • is descriptively named and links to an issue number, i.e. Fixes #123

@luckyklyist
Copy link
Contributor Author

@lindapaiste can you review this pr ?

@lindapaiste
Copy link
Collaborator

lindapaiste commented Sep 22, 2023

I think this is an improvement so I should probably merge it as-is. Just need to load up the branch and run it myself.

Since we are no longer showing different icons on hover the CSS could be cleaned up further, if you want to. We can use the JS to return only the one matching icon and remove all of the display: none; and display: inline-block; switching from the CSS, rather than having both in the HTML and using CSS to hide one.

There are things which are still weird about this QuickAddList which can be handled in future PRs. We have an "are you sure?" dialog before removing a sketch via the normal list, but we don't have any confirmation here. And also there's an aria-label="Descending" which make no sense 🙃. There's a correct label on the parent button so it should be aria-hidden="true" instead. But those things aren't related to the confusing icons.

lindapaiste
lindapaiste previously approved these changes Sep 22, 2023
Copy link
Collaborator

@lindapaiste lindapaiste left a comment

Choose a reason for hiding this comment

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

This is definitely an improvement! It still feels a bit confusing though, especially because the "+" and "x" icons are so visually similar. Do you think it would be better if we used the ✔️ instead of the "x" for items in the collection?

@luckyklyist
Copy link
Contributor Author

I have changed the + to ✔️ .
Screenshot 2023-09-23 at 11 10 42 PM

Copy link
Collaborator

@raclim raclim left a comment

Choose a reason for hiding this comment

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

Thanks for your work on this! :)

@raclim raclim merged commit 259ad33 into processing:develop Jun 3, 2024
1 check passed
@raclim raclim mentioned this pull request Jun 3, 2024
4 tasks
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.

3 participants