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: (Core) set is-disabled class to disabled button #4499

Merged
merged 4 commits into from
Feb 3, 2021

Conversation

artolshansky
Copy link
Contributor

Please provide a link to the associated issue.

Part of #4391

Please provide a brief summary of this pull request.

Set is-disabled class to disabled or aria-disabled="true" buttons.

Please check whether the PR fulfills the following requirements

Documentation checklist:

  • [n/a] update README.md
  • [n/a] Breaking Changes Wiki
  • Documentation Examples
  • Stackblitz works for all examples

Note:

Don't understand why, but this test case not working on the default test case. That's why I've added a new test case.

    it('should add is-disabled class to disabled button', () => {
        element.setAttribute('disabled', null);
        fixture.detectChanges(); // or componentInstance.ngOnChanges();
        componentInstance.buildComponentCssClass();

        let cssClass = componentInstance.buildComponentCssClass().join(' ');
        expect(cssClass).toContain('is-disabled');

        element.removeAttribute('disabled');
        element.setAttribute('aria-disabled', 'true');
        fixture.detectChanges(); // or componentInstance.ngOnChanges();
        componentInstance.buildComponentCssClass();

        cssClass = componentInstance.buildComponentCssClass().join(' ');
        expect(cssClass).toContain('is-disabled');
    });

@artolshansky artolshansky added bug Something isn't working core Core library specific issues labels Jan 27, 2021
@artolshansky artolshansky added this to the Sprint 55 - ariba milestone Jan 27, 2021
@artolshansky artolshansky requested review from a team January 27, 2021 18:30
@artolshansky artolshansky self-assigned this Jan 27, 2021
@netlify
Copy link

netlify bot commented Jan 27, 2021

Deploy preview for fundamental-ngx ready!

Built with commit 8e0a086

https://deploy-preview-4499--fundamental-ngx.netlify.app

@InnaAtanasova
Copy link
Contributor

#4391 is a Stackblitz issue. Are you sure this is the correct issue you are fixing?

@artolshansky
Copy link
Contributor Author

#4391 is a Stackblitz issue. Are you sure this is the correct issue you are fixing?

@InnaAtanasova yeah, that is exactly how I've found this issue (in #4391 this issue described like stackblitz example issue, but it is not an example issue, it's a component issue)

Copy link
Contributor

@valorkin valorkin left a comment

Choose a reason for hiding this comment

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

it's fine

@puru-hk
Copy link
Contributor

puru-hk commented Jan 28, 2021

Tested with the screen reader. It should able to read the disable button

@InnaAtanasova
Copy link
Contributor

#4391 is a Stackblitz issue. Are you sure this is the correct issue you are fixing?

@InnaAtanasova yeah, that is exactly how I've found this issue (in #4391 this issue described like stackblitz example issue, but it is not an example issue, it's a component issue)

thanks for explaining :) I thought it could be a typo.

@artolshansky
Copy link
Contributor Author

Tested with the screen reader. It should able to read the disable button

@puru-hk Button with disabled attribute should not be in the tab order, that's why reader not able to read button and it's by W3 (e.g. you could check a few most popular website, e.g. ebay)

@artolshansky artolshansky merged commit 83c2ca1 into main Feb 3, 2021
@artolshansky artolshansky deleted the fix/core-button-disabled branch February 3, 2021 12:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working core Core library specific issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants