Skip to content

Conversation

Tea-Guru
Copy link

Frontend Pull Request Description

Volunteering fix of accessibility issues for button component.

Checklist

  • [* ] My code follows the style guidelines of this project
  • [* ] I have performed a self-review of my code
  • [ *] I have updated change log
  • [ *] I have been following Definition of done
  • [ *] I have commented my code, particularly in hard-to-understand areas
  • [ *] My changes generate no new lint warnings
  • [ *] New and existing unit tests pass locally and on CI with my changes
  • [ *] Any dependent changes have been merged and published in downstream modules

Screenshots (if applicable)

Additional Context (if necessary)

*ngIf="isBusy"
[ngStyle]="getRippleContainerStyle()"
class="nui-button-ripple-container"
aria-live="polite"
Copy link
Contributor

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"
Copy link
Contributor

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")
Copy link
Contributor

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(() => {
Copy link
Contributor

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(() => {
Copy link
Contributor

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

Copy link
Contributor

@pavlo-poimanov pavlo-poimanov left a comment

Choose a reason for hiding this comment

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

fix comments

@pavlo-poimanov
Copy link
Contributor

also respect the lint to fix the error.

@pavlo-poimanov pavlo-poimanov mentioned this pull request Sep 8, 2025
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants