fix(header-dropdown): click list item to close

[max-umain]

Describe pull-request

Adds div to list of event sources that close the header dropdown.

Adds an event emitter to header-dropdown-list-item that is handled in header-dropdown, where it closes the dropdown.

Adds "pointer-events: none" for relevant inner elements to not interrupt event to close header dropdown. Thank you to @theJohnnyMe for help with the solution :)

Issue Linking:

• Jira: CDEP-3774

How to test

1. Go to https://pr-210.d2vh2lmnzgzrkl.amplifyapp.com/
2. Click the avatar in the top right corner to open the header dropdown
3. Click on the settings icon or text, and notice that now it closes the dropdown

Checklist before submission

• I have added unit tests for my changes (if applicable)
• All existing tests pass
• I have updated the documentation (if applicable)
• Not breaking production behavior
• Behavior available in storybook with documented descriptions (if applicable)
• npm run build-all without errors

Suggested test steps

• Browser testing (Chrome, Safari, Firefox)
• Keyboard operability
• Interactive elements have labels.
• Storybook controls
• Design/controls/props is aligned with other components
• Dark/light mode and variants
• Input fields – values should be displayed properly
• Events

Screenshots

None

Additional context

I'm not sure if this is a very good solution, adding div as a source of event that closes the dropdown. But the way the dropdown is currently closed is handled like this (when the click is done inside the header-dropdown). I imagine a better solution would be to somehow specify the user clicks anywhere in a dropdown row, it closes it, without specifying that it should be a div.

The reason I added div specifically is because in the ticket for this PR, the problem was mentioned to be in React, and after investigating I found that the element that is clicked is a next Link (which is compiled to a div).

[github-actions]

Playwright test results

 789 passed
 1 skipped

Details

 790 tests across 134 suites
 2 minutes, 18 seconds
 40ed740

Skipped tests

src/components/table/table/test/expandable-row-autocollapse/expandable-row-autocollapse.e2e.ts › tds-table-expandable-row-autoCollapse › NEEDS FIXING: expanding one row collapses the others when autoCollapse is true

[aws-amplify-eu-north-1]

This pull request is automatically being deployed by Amplify Hosting (learn more).

Access this pull request here: https://pr-973.d3fazya28914g3.amplifyapp.com

[Lunkan89]

Nice, it works as intended by my testing, just wondering if we need a & button if we add div? since div is the wrapping element? what element are is the source when clicking on the icon or text? should we be more specific, can we check if it is an header-item?

[max-umain]

Nice, it works as intended by my testing, just wondering if we need a & button if we add div? since div is the wrapping element? what element are is the source when clicking on the icon or text? should we be more specific, can we check if it is an header-item?

@Lunkan89 I tested it just now, and having only div causes it to not work when clicking outside of the text or icon

[max-umain]

@Lunkan89 @theJohnnyMe Thanks for your feedback! I updated the code now, to instead emit an event from header-dropdown-list-item. So now, if a list item (row) is clicked, it closes the dropdown.

Not sure if we always want clicks on list items to close the dropdown though... What do you think?

[nathalielindqvist]

@Lunkan89 @theJohnnyMe Thanks for your feedback! I updated the code now, to instead emit an event from header-dropdown-list-item. So now, if a list item (row) is clicked, it closes the dropdown.

Not sure if we always want clicks on list items to close the dropdown though... What do you think?

In the case that a user successfully slots in another dropdown as a list item it might be problematic if the main dropdown closes when a user clicks on the slotted dropdown in an attempt to open it. We might be shooting ourselves in the foot by harshly specifying that all list items closes the dropdown.

[max-umain]

@Lunkan89 @theJohnnyMe Thanks for your feedback! I updated the code now, to instead emit an event from header-dropdown-list-item. So now, if a list item (row) is clicked, it closes the dropdown.
Not sure if we always want clicks on list items to close the dropdown though... What do you think?

In the case that a user successfully slots in another dropdown as a list item it might be problematic if the main dropdown closes when a user clicks on the slotted dropdown in an attempt to open it. We might be shooting ourselves in the foot by harshly specifying that all list items closes the dropdown.

That's a good point. For that reason perhaps it's better with the solution in the first commit... However, perhaps the element that is clicked to open the inner dropdown would also be a div, if it is then it will close the outer dropdown when trying to open the inner. I'll look into this.

Perhaps another solution would be to add a prop to the header-dropdown-list-item component such as closeOnClick which is true by default, but that could be set to false in case one wants to e.g. open an inner dropdown when clicking on the list item. Could this work?

[nathalielindqvist]

I do believe this would work, however I'm wondering about the implementation this would require. From what i can understand the user would have to listen to the newly created event 'closeDropdownFromListItem' on all list items in the header dropdown except the one/few that would potentially open something else, have I understood that correctly? If yes, it would mean a lot of repeating event listeners for a lot of list items. Maybe it would be more desirable to have something the user can opt out of instead, like a "don't close dropdown when this list item is clicked" or similar. Or have I completely misunderstood your implementation? 🙃

[max-umain]

@nathalielindqvist
I'm not sure about the events... I was thinking a prop that, when not passed to the list item, causes the dropdown to close when the list item is clicked. However, if the prop is passed to a list item, it will keep it open. So the default case, when nothing is specified, is to close on click. So my example was a prop called closeOnClick that is true by default, and then the user can set it to false in the few cases it's necessary. But, the opposite would work equally well, such as a prop called keepOpenOnClick that is set to false by default, and can be set to true if necessary :)

[theJohnnyMe]

OK, let's go back to the initial issue. The problem is that when the user clicks directly on the icon or text in the anchor tag, the dropdown does not close. I believe the reason is that we have code like this:

<a href="/settings"><tds-icon name="settings" size="16px" class="hydrated"></tds-icon><div class="tds-u-pl1">Settings</div> </a>

Instead of

<a href="/settings">Settings</a>

So when you click on an Icon or a text, you are clicking on it, not on the a or button that handleSlotteItemClick function expects.

In the browser I did a small test by setting pointer-events: none as CSS rule on tds-icon and div and then there are no issues. Please see if you can do this small change in CSS of the header-dropdown-list-item.scss, I think it should do the trick.
Something like this:
::slotted(a), ::slotted(button) { tds-icon, div { pointer-events: none; } }

Another, no so much desired way, would be to try to add tds-icon and div to the list of eventSources in handleSlottedItemClick.

As I see the user has not requested control to toggle off or on logic for the closing of dropdown so let's keep that discussion out.

[max-umain]

Demo available here now: https://pr-210.d2vh2lmnzgzrkl.amplifyapp.com/

[theJohnnyMe]

Nice one! LGTM

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud