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: Remove rxjs events to avoid duplicate custom events in Angular #566

Merged
merged 2 commits into from
Jun 27, 2024

Conversation

ethanWallace
Copy link
Collaborator

Summary | Résumé

With the v0.22.0 release of the @cdssnc/gcds-components-angular, @Outputs were added to the generated angular components. Unfortunately this was now causing custom events like gcdsClick to be emitted twice.

To fix, I removed Stencil's implementation of events from rxjs to remove the duplicate emitted events.

Example angular component

import {Component} from '@angular/core';

@Component({
  selector: 'app-click-me-gcds',
  template: ` <gcds-button type="button" (gcdsClick)="onClickMe()">Click me!</gcds-button>
    <gcds-pagination type="list" current-page="3" total-pages="20" (gcdsClick)="onClick($event)"></gcds-pagination>
`,
})
export class ClickMeGcdsComponent {
  clickMessage = '';

  onClickMe() {
    console.log('clicked')
  }
  onClick(event: any) {
    event.preventDefault();
    console.log(event.detail)
  }
}

Previously the gcdsClick would be emitted twice when either the button or pagination links were clicked. Now only one event is emitted.

/**
* Custom modification
* - Added conditional to add outputs
*/
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for adding extra comments :)

melaniebmn
melaniebmn previously approved these changes Jun 25, 2024
Copy link
Collaborator

@melaniebmn melaniebmn left a comment

Choose a reason for hiding this comment

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

LGTM

@daine
Copy link
Collaborator

daine commented Jun 25, 2024

FYI I'm still testing this -- should get to it by tonight :)

Comment on lines 3 to 6
/**
* Custom modification
* - Replaced import { fromEvent } from 'rxjs'; with import { EventEmitter } from '@angular/core';
*/
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
/**
* Custom modification
* - Replaced import { fromEvent } from 'rxjs'; with import { EventEmitter } from '@angular/core';
*/
/**
* GCDS Modification
* Replaced
* import { fromEvent } from 'rxjs';
* From: https://github.com/ionic-team/stencil-ds-output-targets/blob/9524c1ce970770e01afb493c292f71a2fe61b14a/packages/angular-output-target/angular-component-lib/utils.ts#L3
*/

Copy link
Collaborator

@daine daine left a comment

Choose a reason for hiding this comment

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

Works pretty well, I tested by using our npm package 0.22.1 to verify that the events were being emitted twice, and then tested this PR and I can confirm that it is no longer firing twice.

Hoping to add more info in the comments about the changes so I added some suggestions in line. Thanks @ethanWallace !

@ethanWallace ethanWallace merged commit 9bf46ac into main Jun 27, 2024
3 checks passed
@ethanWallace ethanWallace deleted the fix/duplicate-angular-events branch June 27, 2024 15:52
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.

3 participants