feat: Add infoButton slot to Field#27030
Closed
behowell wants to merge 8 commits into
Closed
Conversation
Collaborator
📊 Bundle size report
Unchanged fixtures
|
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: 27c3cfe8a95bd26541c7c47ad2fd7fbc8ea332df (build) |
fabricteam
reviewed
Mar 2, 2023
| @@ -4,6 +4,7 @@ import { Checkbox } from '@fluentui/react-checkbox'; | |||
| import { Combobox, Dropdown } from '@fluentui/react-combobox'; | |||
Collaborator
There was a problem hiding this comment.
🕵 fluentuiv9 Open the Visual Regressions report to inspect the 7 screenshots
✅ There was 7 screenshots added, 0 screenshots removed, 1868 screenshots unchanged, 0 screenshots with different dimensions and 0 screenshots with visible difference.
unknown 7 screenshots
| Image Name | Diff(in Pixels) | Image Type |
|---|---|---|
| Field.infoButton+horizontal.default.chromium.png | 0 | Added |
| Field.infoButton+longLabel.default.chromium.png | 0 | Added |
| Field.infoButton+noLabel.default.chromium.png | 0 | Added |
| Field.infoButton+required.default.chromium.png | 0 | Added |
| Field.infoButton+size-large.default.chromium.png | 0 | Added |
| Field.infoButton+size-small.default.chromium.png | 0 | Added |
| Field.infoButton.default.chromium.png | 0 | Added |
Contributor
There was a problem hiding this comment.
Looks like size small + infobutton has some alignment issues. The label is position on the top and the infobutton looks to be lower than the label. Is this expected? Same for large, but not as noticeable.
Collaborator
Perf Analysis (
|
| Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
|---|---|---|---|---|---|
| Avatar | mount | 1275 | 1268 | 5000 | |
| Button | mount | 934 | 930 | 5000 | |
| Field | mount | 1909 | 2004 | 5000 | |
| FluentProvider | mount | 1545 | 1563 | 5000 | |
| FluentProviderWithTheme | mount | 580 | 592 | 10 | |
| FluentProviderWithTheme | virtual-rerender | 544 | 555 | 10 | |
| FluentProviderWithTheme | virtual-rerender-with-unmount | 571 | 583 | 10 | |
| InfoButton | mount | 520 | 515 | 5000 | |
| MakeStyles | mount | 1956 | 1957 | 50000 | |
| Persona | mount | 2841 | 2802 | 5000 | |
| SpinButton | mount | 2271 | 2286 | 5000 |
|
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 8032b68:
|
Contributor
Author
|
Moving back to draft status. Based on discussions with the team, we may instead want to create an |
behowell
marked this pull request as draft
March 8, 2023 21:36
Contributor
Author
|
Abandoning this PR in favor of using the InfoLabel component: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Previous Behavior
Field has no mechanism to correctly add an
InfoButtonafter the label. TheInfoButtonneeds to be outside of the label itself, so that the button's aria-label can be based on the label text.New Behavior
Add two slots to Field:
labelWrapper- an extra div around the label and infoButton. Only rendered when there is an infoButton (otherwise the label is rendered by itself without a wrapper).infoButton- a slot for the InfoButton itself.<InfoButton />as its child. This was done to avoid importing InfoButton directly from the react-field package, which would have caused a large bundle size increase. See this PR exploring that option, and note the very large bundle size increases: feat: Add infoButton slot to Field (discarded implementation option) #27032Update InfoButton to allow it to be configured via context:
InfoButtonContextthat allows some default prop values to be provided via a context.associatedLabelIdprop so that the InfoButton can internally calculate itsaria-labelledby