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

feat: custom single option select component [WD-10960] #912

Merged
merged 1 commit into from
Oct 2, 2024

Conversation

mas-who
Copy link
Collaborator

@mas-who mas-who commented Sep 18, 2024

Done

  • Created a custom single option select component that is searchable and can have its option labels modified.

QA

  1. Run the LXD-UI:
  2. Perform the following QA steps:
    • Navigate to Permissions -> Groups
    • try modify permissions for a group, make sure the interaction with the permission selector is as expected.

@webteam-app
Copy link

@mas-who mas-who force-pushed the customisable-select branch 2 times, most recently from ccf9514 to 7ff3f5f Compare September 18, 2024 14:47
@mas-who
Copy link
Collaborator Author

mas-who commented Sep 18, 2024

@edlerd I updated the implementation for PermissionSelector with the new custom selector component. Haven't modified the option labels for the selectors yet since but have added title attributes to them which was previously not possible. I think we should discuss what's a good design for the permission selectors in terms of option labels and maybe do that as a follow up task? As an example, we can do something like what's shown below (this is not part of this PR at the moment):
Screenshot from 2024-09-18 17-05-26

Lastly, I still need to fix the permission e2e tests, but thought we should have a discussion around about the above topic first since these tests could be affected.

@edlerd
Copy link
Collaborator

edlerd commented Sep 18, 2024

we can do something like what's shown below (this is not part of this PR at the moment):

Interesting idea, but we should not use a chip as non-interactive element as per the design guidelines. They should be interactive, here we need a component that is informational, not interactive.

@mas-who
Copy link
Collaborator Author

mas-who commented Sep 18, 2024

we can do something like what's shown below (this is not part of this PR at the moment):

Interesting idea, but we should not use a chip as non-interactive element as per the design guidelines. They should be interactive, here we need a component that is informational, not interactive.

Aah thanks for the tip! This is currently just a demonstration for what we can do with the new select component 🙂 Not sure if we should update how the option labels are presented as part of this PR though since it might broaden the scope a bit?

@mas-who mas-who force-pushed the customisable-select branch 4 times, most recently from 56a4c5c to 24f5aa0 Compare September 18, 2024 19:00
@mas-who mas-who force-pushed the customisable-select branch 6 times, most recently from c5bff4d to d644827 Compare September 19, 2024 19:07
Copy link
Collaborator

@edlerd edlerd left a comment

Choose a reason for hiding this comment

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

The select works nicely and looks great!

One QA issue:

  • when having many options with some being truncated, then searching for options that have short description, the dropdown width decreases for me

Also a couple of other ideas below.

src/components/select/Select.tsx Outdated Show resolved Hide resolved
src/components/select/Select.tsx Outdated Show resolved Hide resolved
src/components/select/SelectDropdown.tsx Outdated Show resolved Hide resolved
src/components/select/SelectDropdown.tsx Outdated Show resolved Hide resolved
src/components/select/SelectDropdown.tsx Outdated Show resolved Hide resolved
src/util/permissions.tsx Outdated Show resolved Hide resolved
src/components/select/SelectDropdown.tsx Outdated Show resolved Hide resolved
src/sass/_custom_select.scss Outdated Show resolved Hide resolved
src/util/permissions.tsx Outdated Show resolved Hide resolved
src/util/permissions.tsx Outdated Show resolved Hide resolved
@mas-who

This comment was marked as resolved.

@mas-who mas-who force-pushed the customisable-select branch 3 times, most recently from e9cbfd0 to cd383b3 Compare September 23, 2024 14:13
edlerd

This comment was marked as resolved.

@mas-who

This comment was marked as resolved.

@mas-who mas-who force-pushed the customisable-select branch from cd383b3 to b6614e0 Compare September 24, 2024 08:34
@mas-who

This comment was marked as resolved.

@mas-who mas-who force-pushed the customisable-select branch from b6614e0 to a0d2101 Compare September 24, 2024 08:53
@edlerd

This comment was marked as resolved.

@mas-who mas-who force-pushed the customisable-select branch 2 times, most recently from cbdad01 to 921583e Compare September 26, 2024 08:43
@mas-who

This comment was marked as resolved.

@mas-who mas-who force-pushed the customisable-select branch 2 times, most recently from 4c139d7 to 0b17478 Compare September 26, 2024 17:20
Copy link
Collaborator

@edlerd edlerd left a comment

Choose a reason for hiding this comment

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

QA looks mostly good now, found one more issue below.
Several comments for the code.

src/components/select/CustomSelect.tsx Outdated Show resolved Hide resolved
src/components/select/CustomSelect.tsx Outdated Show resolved Hide resolved
src/components/select/CustomSelect.tsx Outdated Show resolved Hide resolved
src/components/select/CustomSelectDropdown.tsx Outdated Show resolved Hide resolved
src/components/select/CustomSelect.tsx Outdated Show resolved Hide resolved
src/components/select/CustomSelectDropdown.tsx Outdated Show resolved Hide resolved
src/pages/permissions/panels/PermissionSelector.tsx Outdated Show resolved Hide resolved
src/pages/permissions/panels/PermissionSelector.tsx Outdated Show resolved Hide resolved
src/util/permissions.tsx Outdated Show resolved Hide resolved
tests/helpers/permission-groups.ts Outdated Show resolved Hide resolved
@mas-who mas-who force-pushed the customisable-select branch 10 times, most recently from 300ab1a to 182ab0b Compare October 2, 2024 04:58
Copy link
Collaborator

@edlerd edlerd left a comment

Choose a reason for hiding this comment

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

Found one more issue on QA, code also seems all right, just one naming issue that confused me.

src/components/select/CustomSelect.tsx Show resolved Hide resolved
src/sass/_custom_select.scss Outdated Show resolved Hide resolved
src/components/select/CustomSelect.tsx Outdated Show resolved Hide resolved
@mas-who mas-who force-pushed the customisable-select branch 3 times, most recently from c969494 to 513ce03 Compare October 2, 2024 08:53
@mas-who mas-who requested a review from edlerd October 2, 2024 08:53
edlerd
edlerd previously approved these changes Oct 2, 2024
Copy link
Collaborator

@edlerd edlerd left a comment

Choose a reason for hiding this comment

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

QA and code LGTM, one idea to simplify a bit more.

src/components/select/CustomSelect.tsx Outdated Show resolved Hide resolved
@mas-who mas-who merged commit 39b15fb into canonical:main Oct 2, 2024
12 checks passed
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.

4 participants