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

Group checkboxes by optgroup label #6

Merged
merged 6 commits into from
Nov 17, 2023
Merged

Conversation

kkarpieszuk
Copy link
Contributor

Fixes #5

See the #5 for the description how it works and how it looks.

Please notice I did not apply any styling for the <h3> grouping elements, @mahdiyazdani if you want to have some styles, feel free to add the ones which will align with the rest of rules you already created. For the purposes of my task I will be re-styling your lib a bit so I will define <h3> styles there anyway.

@kkarpieszuk
Copy link
Contributor Author

@mahdiyazdani if you want to see it in action, switch wpforms to commit https://github.com/awesomemotive/wpforms-plugin/pull/8341/commits/de92bbc36e4406c2ac3a60a0dd858eeadf2845be and visit wpforms > entries > select any form with entries > click gear icon in the top right corner of the table

Copy link
Member

@mahdiyazdani mahdiyazdani left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution. Please find my comments below:

  1. Please consider converting the <h3> label tag to a <span>. This change will make it easier to add additional styles and make customizations where necessary. Heading tags often have unique styles universally and may not be the best choice for controlling and making design adjustments.

  2. Additionally, it would be great if you could align the styles with the Figma design. The dropdown menu is designed with inspiration from the WPForms styling hierarchy.

  3. One more important point is that the group label should be made unselectable to avoid interrupting the user when interacting with checkboxes. You can achieve this by adding user-select: none; to the option group tag in the CSS.

@kkarpieszuk
Copy link
Contributor Author

Additionally, it would be great if you could align the styles with the Figma design.

@mahdiyazdani do you mean the Figma design @mattbrett has created for our rock ( this one ) or do you mean you have some other Figma design exactly for your library somewhere? If the other one, could you share a link? If the first one just please confirm it

@mahdiyazdani
Copy link
Member

@mahdiyazdani do you mean the Figma design @mattbrett has created for our rock ( this one ) or do you mean you have some other Figma design exactly for your library somewhere? If the other one, could you share a link? If the first one just please confirm it

@kkarpieszuk I was referring to the Figma design that you were provided for your task.

@mattbrett
Copy link

If you need various states for the checkboxes, you can find them in the Style Guide as part of the Settings Design Library.

@kkarpieszuk
Copy link
Contributor Author

Thank you @mahdiyazdani ( and @mattbrett ), I changed this PR:

  • changed element from h3 to span
  • added styling
  • added demo to the doc

Mahdi, please continue your review

image

Copy link
Member

@mahdiyazdani mahdiyazdani left a comment

Choose a reason for hiding this comment

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

Thank you @kkarpieszuk 🙌

@mahdiyazdani mahdiyazdani merged commit 4ef6202 into master Nov 17, 2023
3 checks passed
@mahdiyazdani mahdiyazdani deleted the 5-group-checkboxes branch November 17, 2023 17:17
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.

Group checkboxes according to <OPTGROUP>
3 participants