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

Removed highlight from Dropdown components #16789

Merged
merged 18 commits into from
Aug 6, 2024

Conversation

guidari
Copy link
Contributor

@guidari guidari commented Jun 14, 2024

Closes #16512

Removed the selection from first items after the Dropdown is open.
Changed tests to match the expected behaivour.

Testing / Reviewing

  • Test if Combobox, MultiSelect and FilterableMultiselect are not focusing on the first element.

Copy link

netlify bot commented Jun 14, 2024

Deploy Preview for v11-carbon-react ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 22e5a44
🔍 Latest deploy log https://app.netlify.com/sites/v11-carbon-react/deploys/66ac8d791d61f9000949a404
😎 Deploy Preview https://deploy-preview-16789--v11-carbon-react.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Jun 14, 2024

Deploy Preview for carbon-elements ready!

Name Link
🔨 Latest commit 22e5a44
🔍 Latest deploy log https://app.netlify.com/sites/carbon-elements/deploys/66ac8d79c4c04a000876ee8c
😎 Deploy Preview https://deploy-preview-16789--carbon-elements.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Contributor

@preetibansalui preetibansalui left a comment

Choose a reason for hiding this comment

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

Hey @guidari, looks like its not working for filterable multiselect, Please check the attached video.

Screen.Recording.2024-06-19.at.1.01.30.PM.mov

@guidari
Copy link
Contributor Author

guidari commented Jun 19, 2024

@preetibansalui Fixed the arrow click

@guidari guidari requested a review from preetibansalui June 19, 2024 17:37
@preetibansalui
Copy link
Contributor

Thanks @guidari for the changes, Its looking perfect. Just want to confirm one minor thing that I notice in multiselect. If we select something in multiselect and reopen this, It won't have any item as highlighted while for filterable, dropdown and combobox on reopen they have highlighted options. @Kritvi-bhatia17 Can you please confirm if this behaviour is correct?
Attaching video for reference.

Screen.Recording.2024-06-20.at.1.48.06.PM.mov

Copy link
Member

@tay1orjones tay1orjones left a comment

Choose a reason for hiding this comment

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

@preetibansalui that's a configurable option through selectionFeedback, so I think this is okay

Copy link
Contributor

@2nikhiltom 2nikhiltom left a comment

Choose a reason for hiding this comment

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

Looks good ! 🔥

Copy link
Contributor

@Kritvi-bhatia17 Kritvi-bhatia17 left a comment

Choose a reason for hiding this comment

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

Hi @guidari, nice work just a few comments:

  1. I might not have clearly mentioned this in the issue, but when there is no option selected in the combobox, the parent option should have a focus border of 2px. (figma link)

    image

  2. I am unable to verify/crosscheck whether this overlapping focus border is 1px or 2px. Please cross-check. Please verify the same for the multi-select as well.

    Screenshot 2024-07-01 at 5 12 39 PM
  3. The focus border is supposed to be removed from the child option when it is not selected. In this case, as one option is already selected, the focus border should also appear on the child option (the way it is currently now), similar to what you have done for the filterable-multiselect. (example link)

    Screenshot 2024-07-01 at 5 16 16 PM
  4. In the filterable-multiselect, I am unsure whether the last selected option interacted with should have the focus state or if it should be the first option that appears when opening the dropdown. For now, let's continue with the current interaction as seen in the current storybook.

Please cross-check these minor points with the fluid states as well.

Let me know if you have any doubts. Thank you, Gui! 🔥

@guidari guidari requested a review from Kritvi-bhatia17 July 12, 2024 12:51
Copy link
Contributor

@Kritvi-bhatia17 Kritvi-bhatia17 left a comment

Choose a reason for hiding this comment

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

Hi Gui!! Thanks for fixing those minor points 🙌🏻.
Can you please look at the following points:

  1. Can you please reconfirm if this is resolved now? I am unable to see it on my end. The same issue is occurring for fluid multi-select.

    image

    3. The focus border is supposed to be removed from the child option when it is not selected. In this case, as one option is already selected, the focus border should also appear on the child option (the way it is currently now), similar to what you have done for the filterable-multiselect. (example link)

    Screenshot 2024-07-01 at 5 16 16 PM

  1. 'When no option is selected in the combobox, the parent option should have a 2px focus border.' This same rule should be applied to the fluid combo box as well.

    Screenshot 2024-07-13 at 1 32 35 PM

  1. In both multi-select and filterable multi-select, when I select an option, click the cross, and then open the dropdown again, the focus state reappears. I'm not sure why this is happening. Can you please check this?
Screen.Recording.2024-07-13.at.1.58.02.PM.mov

@Kritvi-bhatia17
Copy link
Contributor

Although this is not related to this issue, when I click on multi-select, the component appears in the middle instead of the top left like all other components. Any idea why this bug is occurring?

Screenshot 2024-07-13 at 2 09 50 PM

@guidari
Copy link
Contributor Author

guidari commented Jul 15, 2024

Fixed the issues @Kritvi-bhatia17

Although this is not related to this issue, when I click on multi-select, the component appears in the middle instead of the top left like all other components. Any idea why this bug is occurring?

It is only for testing purpouse. So the user can test the Floating UI when the autoAlign is set to true

@guidari guidari requested a review from Kritvi-bhatia17 July 15, 2024 19:53
Copy link
Contributor

@Kritvi-bhatia17 Kritvi-bhatia17 left a comment

Choose a reason for hiding this comment

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

Amazing, Gui! It is working perfectly fine now. 🔥
Thanks for patiently working on it. 🙌🏻

@guidari guidari self-assigned this Jul 24, 2024
@thyhmdo
Copy link
Member

thyhmdo commented Jul 24, 2024

Sorry, I came late to this issue. Just wanted to ask if this is correct for the Invalid and Warning states.

image

@guidari guidari enabled auto-merge August 1, 2024 20:21
Copy link
Contributor

@preetibansalui preetibansalui left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the changes 🚀

@guidari guidari added this pull request to the merge queue Aug 6, 2024
github-merge-queue bot pushed a commit that referenced this pull request Aug 6, 2024
* fix: removed highlight from first item

* test: fixed tests to match new behaviour

* test: fixed fluid components tests

* fix: fixed click on arrow

* fix: fixed focus state

* fix: fixed filterable multiselect focus

* fix: fixed focus state for combobox

* fix: fixed scss

* fix: fixed multiselect focus state

* fix: removed console.log

* fix: fixex arrowDown to open the menu

* fix: fixed tests

* fix: fixed issues

---------

Co-authored-by: Kritvi <[email protected]>
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 6, 2024
@Kritvi-bhatia17 Kritvi-bhatia17 added this pull request to the merge queue Aug 6, 2024
Merged via the queue into carbon-design-system:main with commit 5998863 Aug 6, 2024
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[Dropdown: Combobox & Multiselect]: Fixing focus state dev implementation
6 participants