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

fix: added cursor pointer for checkbox and pre-filled list #1793

Merged
merged 7 commits into from
Oct 16, 2024
Merged

fix: added cursor pointer for checkbox and pre-filled list #1793

merged 7 commits into from
Oct 16, 2024

Conversation

Sam-Phillemon9493
Copy link
Contributor

What does it do?
Fixed the Media Library's missing cursor pointers issue on the filters

Why is it needed?
To fix issue #1791: Media Library missing cursor pointers on the filters

How to test it?

  • Open the Media Library
  • Hover over the filters, it should show the cursor pointers.

Related issue(s)/PR(s)
#1791

@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Sep 27, 2024 •
edited
Loading

🦋 Changeset detected

Latest commit: 86b4d50

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@strapi/design-system Patch
@strapi/icons Patch
@strapi/ui-primitives Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@vercel Vercel
Copy link

vercel bot commented Sep 27, 2024 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
design-system ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 16, 2024 10:53am

@Sam-Phillemon9493 Sam-Phillemon9493 marked this pull request as ready for review September 27, 2024 10:16
@Sam-Phillemon9493
Copy link
Contributor Author

Hi @remidej , As discussed in this PR 21460, here’s the new PR for the issue. Please let me know if you have any feedback on this change.

remidej
remidej reviewed Sep 27, 2024
View reviewed changes
Copy link
Contributor

@remidej remidej 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 moving the PR 🙌

packages/design-system/src/components/Checkbox/Checkbox.tsx Outdated Show resolved Hide resolved
remidej
remidej reviewed Oct 1, 2024
View reviewed changes
packages/design-system/src/components/Checkbox/Checkbox.tsx Outdated
@@ -96,6 +98,7 @@ const CheckboxRoot = styled(Checkbox.Root)`
height: 100%;
min-width: 44px;
min-height: 44px;
pointer-events: none;
Copy link
Contributor

Choose a reason for hiding this comment

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

We can't do that because it's not just a visual change about what the cursor looks like, it makes the area not clickable. And as indicated in the comment above we do want to increase the touch target to make it easier to click, especially on mobile.

So I would recommend changing the approach and removing cursor: pointer from the root, and instead only applying it to the icon. Then you wouldn't need to change this pseudo element

Copy link
Contributor Author

@Sam-Phillemon9493 Sam-Phillemon9493 Oct 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@remidej , I tried applying to the icon, but it doesn’t show in the corners of the checkbox. To resolve this, I added it to the Checkbox Indicator, which ensures that it now encompasses the entire area.

Please let me know if you have any additional thoughts or suggestions!

packages/design-system/src/components/Select/SelectParts.tsx Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

I checked with @lucasboilly and he also wants cursor pointer on the select's options, could you add that too?

Copy link
Contributor Author

@Sam-Phillemon9493 Sam-Phillemon9493 Oct 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@remidej , I have implemented the changes.

@remidej
Copy link
Contributor

remidej commented Oct 7, 2024

Sorry I meant to ping @lucasboilly for a final check

@lucasboilly
Copy link

Hey @Sam-Phillemon9493, it looks great! Thanks for your contribution!

@remidej remidej added pr: enhancement This PR adds or updates some part of the codebase or features source: design-system relates to design-system package labels Oct 8, 2024
remidej
remidej previously approved these changes Oct 8, 2024
View reviewed changes
remidej
remidej reviewed Oct 8, 2024
View reviewed changes
Copy link
Contributor

@remidej remidej left a comment

Choose a reason for hiding this comment

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

I forgot this just needs a changeset (something we use for auto geenrated releases and changelogs), could you add one?

yarn release:add and mark it as a patch

@Sam-Phillemon9493
Copy link
Contributor Author

I forgot this just needs a changeset (something we use for auto geenrated releases and changelogs), could you add one?

yarn release:add and mark it as a patch

Hi @remidej , I've added a changeset. Please let me know if anything else is needed.

@remidej remidej merged commit fae2bf8 into strapi:main Oct 16, 2024
11 of 12 checks passed
@github-actions github-actions bot mentioned this pull request Oct 16, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Aurelsicoko Aurelsicoko Aurelsicoko left review comments

@remidej remidej remidej approved these changes

Assignees

@remidej remidej

Labels
community pr: enhancement This PR adds or updates some part of the codebase or features source: design-system relates to design-system package
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Sam-Phillemon9493 @remidej @lucasboilly @Aurelsicoko @oiorain