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(gcds-button): Prevent gcdsClick event if button is disabled #724

Merged
merged 5 commits into from
Jan 10, 2025

Conversation

ethanWallace
Copy link
Collaborator

Summary | Résumé

As mentioned in #720, the gcdsClick event will still fire even after the button has been disabled. Previous work has been made that will prevent HTML form submissions if the button is disabled but preventing the gcdsClick event from emitting when disabled will allow easier use in applications.

melaniebmn
melaniebmn previously approved these changes Jan 7, 2025
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

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.

Is it possible to add a test for the fix?

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 Jan 9, 2025

While testing, I'm able to confirm that it does not fire gcdsClick anymore, however I think I'm still able to click on a disabled button?

Tested by adding this script on the index.html page in src/index.html:

    const button = document.getElementById('myButton');
    button.addEventListener('click', () => {
      console.log("Button was clicked!");
    });

similar to how I verify that gcdsClick doesn't get emitted

    const container = document.querySelector('body'); // or any specific container

    container.addEventListener('gcdsClick', (e) => {
      console.log(`Event gcdsClick was emitted by`, e.target, 'with details:', e.detail);
    });

here is my html:

      <gcds-heading tag="h2">Buttons</gcds-heading>
      <gcds-button button-role="primary">Primary</gcds-button>
      <gcds-button button-role="secondary">Secondary</gcds-button>
      <gcds-button button-role="danger">Danger</gcds-button>
      <gcds-button id="myButton" disabled>Click me!</gcds-button>

Screenshot 2025-01-08 at 11 22 33 PM

🤔 any ideas why?

@ethanWallace
Copy link
Collaborator Author

@daine With the HTML that is rendered in the gcds-button component, we chose to use aria-disabled instead of the disabled attribute to allow users to still focus the button and not have the button disappear from the document flow completely. We do some additional logic on our side to make the button behave as it would if it was just using the disabled attribute but that does mean the native HTML events will still happen because the button is still kind of recognized as active by the browser.

@ethanWallace ethanWallace merged commit 1cd508a into main Jan 10, 2025
3 checks passed
@ethanWallace ethanWallace deleted the fix/disabled-button-gcdsClick-event branch January 10, 2025 20:43
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