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

updated to have a click event emitter in HeaderAction #2699

Closed
wants to merge 1 commit into from

Conversation

ParkerMIBM
Copy link

Closes #2698

Since moving to the base icon button, the click event does not propagate up the chain. This adds the click event as an Output on the BaseIconButton and the emitter within the onClick function in the UIShellModule Header Action Component

Changelog

Changed

  • BaseIconButton to have an Output
  • Header Action click feature returned

@ParkerMIBM ParkerMIBM requested a review from a team as a code owner September 19, 2023 23:07
@netlify
Copy link

netlify bot commented Sep 19, 2023

Deploy Preview for carbon-components-angular ready!

Name Link
🔨 Latest commit 0e28c33
🔍 Latest deploy log https://app.netlify.com/sites/carbon-components-angular/deploys/650a29ab610a4d00084ed4ab
😎 Deploy Preview https://deploy-preview-2699--carbon-components-angular.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.

@github-actions
Copy link

github-actions bot commented Sep 19, 2023

DCO Assistant Lite bot All contributors have signed the DCO.

@ParkerMIBM
Copy link
Author

I have read the DCO document and I hereby sign the DCO.

@@ -60,5 +60,6 @@ export class HeaderAction extends BaseIconButton {
this.active = !this.active;
this.selected.emit(this.active);
this.activeChange.emit(this.active);
this.click.emit(this.active);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is redundant. There is a selected and an activeChange event emitted with the same data. Instead listening for click event, just listen for selected or activeChange event. You can even double bind a variable with [active].

Copy link
Author

Choose a reason for hiding this comment

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

Then is that not an upgrade action that should be documented?
The (click) property on header-action-component seems like it is unused now, and code in the field binding to click breaks when moving from v4 to v5.

Copy link
Contributor

@Akshat55 Akshat55 Sep 28, 2023

Choose a reason for hiding this comment

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

Just checked, it never emitted a click event. Only the events I mentioned above were emitted. You can view the v10 files here: https://github.com/carbon-design-system/carbon-components-angular/blob/v10/src/ui-shell/header/header-action.component.ts

You probably were able to capture the event because the click event was bubbled. We stop propagation in icon button so the event doesn't bubble up and cannot be captured. It was never suggested that click event should've been used in our documentation or storybook hence it was not mentioned in our documentation.

@Akshat55
Copy link
Contributor

@ParkerMIBM I'm closing this PR as we have alternatives available.

@Akshat55 Akshat55 closed this Oct 12, 2023
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.

CDS Header Action Click property not propagating
2 participants