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

fix(action-group): fixes action group click event which was not selecting action button when they are child elements of action group #3292

Merged
merged 6 commits into from Jun 26, 2023

Conversation

sonirajan
Copy link
Contributor

Motivation and context

I’m trying to use sp-action-group component with sp-action-button as child components. When I try to select/click one of the sp-action-button, it calls handleClick event. Here, event.target is always span instead of parent sp-action-button. And, because of that target.value is always undefined and returns and does nothing.

Technical detail: Retargeting is not occuring because the event occurs on a slotted element inside action button, that physically lives in the light DOM. And because of that event.target is always span instead of sp-action-button. To fix that, we used event.composedPath() and compares if any html element in path is action button that is part of this.buttons.

Related issue(s)

#3048
Slack discussion

How has this been tested?

ccx-commenting uses filter popover to filter comments where we have multiple action groups(as various filters) with action buttons as child elements. When we tried to use SWC (instead of RS2), clicking various filters (clicking action button) was not selecting action button(filter not being applied). I tested fix directly in commenting component.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Chore (minor updates related to the tooling or maintenance of the repository, does not impact compiled assets)

Checklist

  • I have signed the Adobe Open Source CLA.
  • My code follows the code style of this project.
  • If my change required a change to the documentation, I have updated the documentation in this pull request.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • I have reviewed at the Accessibility Practices for this feature, see: Aria Practices

Best practices

This repository uses conventional commit syntax for each commit message; note that the GitHub UI does not use this by default so be cautious when accepting suggested changes. Avoid the "Update branch" button on the pull request and opt instead for rebasing your branch against main.

@Westbrook
Copy link
Collaborator

I wonder if there is a CSS only solution to this? Possibly:

::slotted(*) {
    pointer-events: none !important;
}

🤔...

@sonirajan
Copy link
Contributor Author

I wonder if there is a CSS only solution to this? Possibly:

::slotted(*) {
    pointer-events: none !important;
}

🤔...

Should I use it here and test it out?

@Westbrook
Copy link
Collaborator

That file is mechanically generated, so you'll need to include it here, to test. 🤞🏼

@sonirajan
Copy link
Contributor Author

sonirajan commented Jun 19, 2023

Hi Westbook, Hope you had nice days off :)
Regarding your last comment, CSS solution is not working. event.target is sp-action-group and target.value is undefined, so handleClick just returns. Also, there is no hover effect on action buttons.
image

@sonirajan
Copy link
Contributor Author

I did more research and found why above CSS won't work I think. The ::slotted() represents any element that has been placed into a slot inside an HTML template. In ActionGroup, all action buttons are placed inside one slot. So, no pointer event is attached to any action button when we go with CSS solution above. And so, when we click any button of Action Group, event.target is ActionGroup itself in handleClick and it will just return and do nothing.
image

@Westbrook
Copy link
Collaborator

I see. My intent was for this CSS rule to be added to the Action Button, not the Group. The hope there being that no matter what was slotted into the Action Button the host would receive all events. If that doesn’t work, we can move forward with you approach in JS.

@sonirajan
Copy link
Contributor Author

sonirajan commented Jun 26, 2023

I see. My intent was for this CSS rule to be added to the Action Button, not the Group. The hope there being that no matter what was slotted into the Action Button the host would receive all events. If that doesn’t work, we can move forward with you approach in JS.

CSS fix on Action Button is working fine. Thanks for the hint.

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.

None yet

2 participants