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: CheckboxField to set a generated ID on the input, to match the label's htmlFor #25079

Merged
merged 2 commits into from
Oct 7, 2022

Conversation

behowell
Copy link
Contributor

@behowell behowell commented Oct 5, 2022

Current Behavior

Field sets the input's id to undefined if there is no field label. CheckboxField is unique in that it doesn't use the field label, but rather an internal label built into Checkbox. The Field's id={undefined} gets passed to the Checkbox component, which prevents it from setting a generated ID on the input element.

New Behavior

  • Have field always set a generated ID on the input component, even if there is no field label.
  • Refactor useField to not set input props to undefined if they aren't relevant (instead they will be unset in that case)
  • Add tests for these changes.

Related Issue(s)

@behowell behowell self-assigned this Oct 5, 2022
@github-actions github-actions bot added this to the October Project Cycle Q4 2022 milestone Oct 5, 2022
@fabricteam
Copy link
Collaborator

fabricteam commented Oct 5, 2022

📊 Bundle size report

Unchanged fixtures
Package & Exports Size (minified/GZIP)
react-components
react-components: Button, FluentProvider & webLightTheme
62.781 kB
17.574 kB
react-components
react-components: Accordion, Button, FluentProvider, Image, Menu, Popover
189.227 kB
52.474 kB
react-components
react-components: FluentProvider & webLightTheme
33.4 kB
11.008 kB
react-portal-compat
PortalCompatProvider
5.857 kB
1.978 kB
🤖 This report was generated against a8e81b5296f1551e74010c1d2ad9d37c6fd48306

@codesandbox-ci
Copy link

codesandbox-ci bot commented Oct 5, 2022

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 724d581:

Sandbox Source
@fluentui/react 8 starter Configuration
@fluentui/react-components 9 starter Configuration
agitated-jepsen-jesykv Issue #25072
romantic-galileo-k07sbw Issue #25072

@size-auditor
Copy link

size-auditor bot commented Oct 5, 2022

Asset size changes

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

Baseline commit: a8e81b5296f1551e74010c1d2ad9d37c6fd48306 (build)

@fabricteam
Copy link
Collaborator

fabricteam commented Oct 5, 2022

Perf Analysis (@fluentui/react-components)

No significant results to display.

All results

Scenario Render type Master Ticks PR Ticks Iterations Status
Avatar mount 1620 1596 5000
Button mount 1137 1166 5000
FluentProvider mount 2000 1950 5000
FluentProviderWithTheme mount 765 775 10
FluentProviderWithTheme virtual-rerender 703 710 10
FluentProviderWithTheme virtual-rerender-with-unmount 779 739 10
MakeStyles mount 2315 2310 50000
SpinButton mount 3001 3119 5000

@behowell behowell marked this pull request as ready for review October 6, 2022 01:06
@behowell behowell requested a review from a team as a code owner October 6, 2022 01:06
@behowell behowell merged commit c78ae4c into microsoft:master Oct 7, 2022
@behowell behowell deleted the field/checkbox-htmlfor branch October 7, 2022 17:48
marcosmoura added a commit to marcosmoura/fluentui that referenced this pull request Oct 10, 2022
* master: (23 commits)
  Revert "chore: screener-run workflow should report to PR (microsoft#25144)" (microsoft#25145)
  chore: screener-run workflow should report to PR (microsoft#25144)
  applying package updates
  fix: The Tooltip should not hide if it gets keyboard focus (microsoft#25138)
  fix: Tooltip should not hide if an element inside it gets focused (microsoft#25140)
  Create react-migration-v8-v9 with shims and stories (microsoft#25121)
  fix: CheckboxField to set a generated ID on the input, to match the label's htmlFor (microsoft#25079)
  feat: Overflow menu should be registered in overflowManager (microsoft#25091)
  fix: version-bump generator removes beachball disallowedChangeType config (microsoft#25130)
  fix: new overflow items should only be enqueued while observing (microsoft#25122)
  fix(script): allow runPublished call from CLI (microsoft#25127)
  feat: add vertical variation for toolbar (microsoft#24940)
  ProgressField implementation and stories (microsoft#25103)
  fix: Dropdown icon layout with no placeholder/value (microsoft#25049)
  chore: add a bundle size fixture (Button, Provider & theme) (microsoft#25113)
  feat: Adding subtle transition between states on Button components (microsoft#25106)
  Revert "chore: screener-run workflow should report to PR (microsoft#25096)" (microsoft#25115)
  chore: screener-run workflow should report to PR (microsoft#25096)
  fix(react-dialog): aria-* properties should be reassignable (microsoft#25092)
  fix(scripts): don't run publish if web-components are affected (microsoft#25095)
  ...
NotWoods pushed a commit to NotWoods/fluentui that referenced this pull request Nov 18, 2022
…abel's htmlFor (microsoft#25079)

* Have field _always_ set a generated ID on the input component, even if there is no field label.
* Refactor useField to not set input props to `undefined` if they aren't relevant (instead they will be unset in that case)
* Add tests for these changes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: CheckboxField component does not automatically generate "id" attribute when not given via props
3 participants