-
Notifications
You must be signed in to change notification settings - Fork 25
Fix button issues #774
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
base: main
Are you sure you want to change the base?
Fix button issues #774
Conversation
*ngIf="isBusy" | ||
[ngStyle]="getRippleContainerStyle()" | ||
class="nui-button-ripple-container" | ||
aria-live="polite" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aria-busy="true"
aria-live="polite"
Don't use aria-label or aria-labelledby on a span or div unless its given a role.
aria-label=Loading is not ok in this case since this element do not have content but only visual indicator of busy
[ngStyle]="getRippleContainerStyle()" | ||
class="nui-button-ripple-container" | ||
aria-live="polite" | ||
aria-atomic="true" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aria-atomic true I wouldn't use that because it force screen readers to read it contents
@Input() public ariaLabel: string = ""; | ||
|
||
/** Sets aria-disabled for disabled state programmatic indication */ | ||
@HostBinding("attr.aria-disabled") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be on the control of the clients but not handle form the nova side
this is not correct because screen readers already rely on the native
https://ripple.watermarkinsights.com/blog/a-few-remarks-on-using-aria/
only valid case is next:
but as you can see it is under control of client code not nova.
|
||
private validateIconOnlyButtonAccessibility(): void { | ||
// Check if button will be icon-only and validate accessibility | ||
setTimeout(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why set timeout?
merge(keyUp$, this.ngUnsubscribe) | ||
) | ||
) | ||
.subscribe(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
subscribe in subscribe wow that is awesome code ! great ... kiding
this may lead to memory leaks and the code overcomplicated which makes it un readable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix comments
also respect the lint to fix the error. |
Frontend Pull Request Description
Volunteering fix of accessibility issues for button component.
Checklist
Screenshots (if applicable)
Additional Context (if necessary)