Skip to content

Initial spin-button a11y commit #24075

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
Merged
spmonahan merged 11 commits into from
Aug 22, 2022
Merged

Initial spin-button a11y commit #24075

spmonahan merged 11 commits into from
Aug 22, 2022

Conversation

@jxlz
Copy link
Contributor

@jxlz jxlz commented Jul 26, 2022

Keyboard state diagram, and open questions on accessibility interaction and specification highlighted.

@jxlz
Initial spin-button a11y commit
5f93f24 
Keyboard state diagram, and open questions on accessibility interaction and specification highlighted.
@jxlz jxlz requested review from a team and spmonahan as code owners July 26, 2022 01:36
@jxlz jxlz marked this pull request as draft July 26, 2022 01:38
@codesandbox-ci
Copy link

codesandbox-ci bot commented Jul 26, 2022 •
edited
Loading

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit df44603:

Sandbox Source
@fluentui/react 8 starter Configuration
@fluentui/react-components 9 starter Configuration

@fabricteam
Copy link
Collaborator

fabricteam commented Jul 26, 2022 •
edited
Loading

📊 Bundle size report

Unchanged fixtures
Package & Exports Size (minified/GZIP)
react-components
react-components: Accordion, Button, FluentProvider, Image, Menu, Popover		 189.031 kB
51.967 kB
react-components
react-components: FluentProvider & webLightTheme		 32.895 kB
10.778 kB
react-spinbutton
SpinButton		 43.899 kB
12.362 kB
🤖 This report was generated against 491945715ffc331e66cf0e371e6e8349d9a9499f

@size-auditor
Copy link

size-auditor bot commented Jul 26, 2022 •
edited
Loading

Asset size changes

Size Auditor did not detect a change in bundle size for any component!

Baseline commit: 491945715ffc331e66cf0e371e6e8349d9a9499f (build)

@fabricteam
Copy link
Collaborator

fabricteam commented Jul 26, 2022 •
edited
Loading

Perf Analysis (@fluentui/react-components)

No significant results to display.

All results

Scenario Render type Master Ticks PR Ticks Iterations Status
Avatar mount 1184 1118 5000
Button mount 874 960 5000
FluentProvider mount 1395 1412 5000
FluentProviderWithTheme mount 557 556 10
FluentProviderWithTheme virtual-rerender 522 605 10
FluentProviderWithTheme virtual-rerender-with-unmount 627 633 10
MakeStyles mount 1836 1763 50000
SpinButton mount 2306 2278 5000

smhigley
smhigley reviewed Jul 26, 2022
View reviewed changes
Copy link
Contributor

@smhigley smhigley left a comment

Choose a reason for hiding this comment

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

I left a bunch of comments, though not all of them need to be addressed in this PR -- we can add future PRs with more specifics to flesh out sections.

packages/react-components/react-spinbutton/src/stories/SpinButton/SpinButtonAccessibility.md Outdated
#### Child content restrictions

The following child content is allowed:
1. Text
Copy link
Contributor

@smhigley smhigley Jul 26, 2022

Choose a reason for hiding this comment

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

I believe we actually do not allow any children, since the primary slot is an <input> element.

We could mention that the up/down buttons can have children, though.

Copy link
Contributor Author

@jxlz jxlz Jul 26, 2022

Choose a reason for hiding this comment

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

Other than carats, what other children are allowed?

Copy link
Contributor

@spmonahan spmonahan Aug 5, 2022

Choose a reason for hiding this comment

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

Other than carats, what other children are allowed?

Technically, anything that can be a child of a <button>. In practice it should probably be restricted to a 16px icon from @fluentui/react-icons as other content will likely break the visual design.

jxlz and others added 6 commits July 26, 2022 16:07
@jxlz jxlz requested review from smhigley and spmonahan August 4, 2022 23:09
spmonahan
spmonahan reviewed Aug 5, 2022
View reviewed changes
Copy link
Contributor

@spmonahan spmonahan left a comment

Choose a reason for hiding this comment

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

Looking good!

jxlz and others added 2 commits August 15, 2022 10:38
@jxlz @spmonahan
Apply suggestions from code review
62d0b1a 
Co-authored-by: Sean Monahan <[email protected]>
@jxlz
Update SpinButtonAccessibility.md
ce0b1a6 
Replace placeholders for known issues and required props
@jxlz jxlz marked this pull request as ready for review August 15, 2022 18:13
spmonahan
spmonahan approved these changes Aug 17, 2022
View reviewed changes
Copy link
Contributor

@spmonahan spmonahan left a comment

Choose a reason for hiding this comment

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

Looks good! Just a few wording suggestions.

jxlz and others added 2 commits August 18, 2022 09:51
@jxlz @spmonahan
Update packages/react-components/react-spinbutton/src/stories/SpinBut…
7dd91d5 
…ton/SpinButtonAccessibility.md

Co-authored-by: Sean Monahan <[email protected]>
@jxlz @spmonahan
Update packages/react-components/react-spinbutton/src/stories/SpinBut…
df44603 
…ton/SpinButtonAccessibility.md

Co-authored-by: Sean Monahan <[email protected]>
@jxlz jxlz closed this Aug 19, 2022
@jxlz jxlz reopened this Aug 19, 2022
@spmonahan spmonahan merged commit 7f7f925 into microsoft:master Aug 22, 2022
@smhigley smhigley mentioned this pull request Sep 22, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@spmonahan spmonahan spmonahan approved these changes

@smhigley smhigley Awaiting requested review from smhigley

Assignees

@TristanWatanabe TristanWatanabe

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@jxlz @fabricteam @smhigley @spmonahan @TristanWatanabe