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.

Sign up for GitHub

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

Jump to bottom

Rewrite Button to Typescript #2984

Merged
merged 12 commits into from
Aug 22, 2021
Merged

Rewrite Button to Typescript #2984

merged 12 commits into from
Aug 22, 2021

Conversation

davwheat
Copy link
Member

Progresses #3533, #3364

Changes proposed in this pull request

Button:

  • Prefer aria-label over title
  • Don't duplicate button content to title attribute
    • This has no effect on a11y. Content of nodes is preferred over title attributes by screen readers.
  • Warn in debug mode if button has no accessible content
  • Use modern JS/TS syntax (||=, spread, etc)

Others:

  • Add debug warning helper: fires console.warn, but only when the forum is in debug mode. Can help to inform extension developers of possible issues with their JS code.
  • Update TextEditorButton to work with changes that prefer aria-label

Reviewers should focus on

Is anything else affected by the preferring of aria-label as opposed to title? Most likely components that deeply interact with Button and those which extend it.

Confirmed

  • Frontend changes: tested on a local Flarum installation.
  • Backend changes: tests are green (run composer test).

Required changes

  • Related documentation PR: (Remove if irrelevant)
  • Related core extension PRs: (Remove if irrelevant)

@davwheat davwheat added type/cleanup type/accessibility Issues relating to accessibility (keyboard navigation, screenreaders, text contrast, etc.) labels Jul 21, 2021
@davwheat davwheat added this to the 1.1 milestone Jul 21, 2021
@davwheat davwheat self-assigned this Jul 21, 2021
tankerkiller125
tankerkiller125 suggested changes Jul 21, 2021
View reviewed changes
Copy link
Contributor

@tankerkiller125 tankerkiller125 left a comment

Choose a reason for hiding this comment

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

I think the warning message requires a very slight re-wording so that it isn't confusing, otherwise it looks good to me.

js/src/common/components/Button.tsx Outdated Show resolved Hide resolved
@dsevillamartin
Copy link
Member

The warning will be logged every time the button redraws... I think it'd be good to only do it once per element, but that'd probably be outside the scope of this PR.

@davwheat davwheat requested a review from tankerkiller125 July 21, 2021 19:39
@davwheat
Copy link
Member Author

I couldn't think of a better way to handle that, to be honest, without risking the checks not working correctly. Things like this.element aren't defined in oninit as far as I'm aware, meaning we can't check if the Tooltip component has set an aria-label attribute.

@dsevillamartin
Copy link
Member

I couldn't think of a better way to handle that, to be honest, without risking the checks not working correctly. Things like this.element aren't defined in oninit as far as I'm aware, meaning we can't check if the Tooltip component has set an aria-label attribute.

It's defined in Component.oncreate(), I believe. Warning in oncreate should do the trick.

@davwheat davwheat force-pushed the dw/ts-rewrite/button branch from 71f42f0 to b5cd804 Compare July 24, 2021 19:40
davwheat and others added 7 commits July 30, 2021 19:58
@davwheat
Rename Button file
3d10902
@davwheat
Convert to TS
b9b4548
@davwheat
Add debug warning helper
fb05d46 
Fires `console.warn`, but only when the forum is in debug mode. Can help to inform extension developers of possible issues with their JS code.
@davwheat
Simplify button content template
2fbf780
@davwheat
Rewrite Button component
935d5a8 
- Prefer `aria-label` over `title`
- Don't duplicate button content to `title` attribute
- Warn in debug mode if button has no accessible content
- Use modern JS/TS syntax (`||=`, spread, etc)
@davwheat
Update to work with new Button component
ba68401
@davwheat @tankerkiller125
Update warning
7f6f4b8 
Co-authored-by: Matt Kilgore <tankerkiller125@gmail.com>
@davwheat davwheat force-pushed the dw/ts-rewrite/button branch from b5cd804 to 7f6f4b8 Compare July 30, 2021 18:59
@davwheat davwheat requested a review from askvortsov1 August 20, 2021 21:31
askvortsov1
askvortsov1 approved these changes Aug 20, 2021
View reviewed changes
js/src/common/components/Button.tsx Outdated Show resolved Hide resolved
js/src/common/helpers/fireDebugWarning.ts Outdated Show resolved Hide resolved
@davwheat davwheat merged commit 3cf19dd into master Aug 22, 2021
@davwheat davwheat deleted the dw/ts-rewrite/button branch August 22, 2021 19:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@askvortsov1 askvortsov1 askvortsov1 approved these changes

@dsevillamartin dsevillamartin dsevillamartin left review comments

@tankerkiller125 tankerkiller125 tankerkiller125 approved these changes

@SychO9 SychO9 Awaiting requested review from SychO9

Assignees

@davwheat davwheat

Labels
type/accessibility Issues relating to accessibility (keyboard navigation, screenreaders, text contrast, etc.) type/cleanup
Projects
None yet
Milestone
1.1
Development

Successfully merging this pull request may close these issues.
4 participants
@davwheat @dsevillamartin @tankerkiller125 @askvortsov1