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(FormExample): Update fields ID and add success indicator by default when field required #13568

Merged
merged 29 commits into from
Jun 12, 2020

Conversation

assuncaocharles
Copy link
Contributor

@assuncaocharles assuncaocharles commented Jun 11, 2020

Pull request checklist

  • Addresses an existing issue: Fixes #0000
  • Include a change request file using $ yarn change

BREAKING CHANGES

This PR updates input/label ID's to correctly match and add successIndicator by default when the field is required.

Inputs when required will by default show success indicator when fulfilled, to prevent it and have the previously behavior just use <Input required showSuccessIndicator={false} />or

const fields =  [{
    label: 'First name',
    name: 'firstName',
    id: 'first-name-error',
    key: 'first-name',
    errorMessage: 'You can not fix this error',
    required: true,
    control: { 
         as: Input,
         showSuccessIndicator: false
    }
  }]

Focus areas to test

(optional)

@assuncaocharles assuncaocharles changed the title feat(FormExample): Update fields ID and add success indicator by default when field required fix(FormExample): Update fields ID and add success indicator by default when field required Jun 11, 2020
@msft-github-bot
Copy link
Contributor

msft-github-bot commented Jun 11, 2020

Perf Analysis

No significant results to display.

All results

Scenario Render type Master Ticks PR Ticks Iterations Status
BaseButton mount 948 961 5000
ButtonNext mount 477 455 5000
Checkbox mount 1711 1674 5000
CheckboxBase mount 1458 1433 5000
CheckboxNext mount 1752 1736 5000
ChoiceGroup mount 5298 5201 5000
ComboBox mount 982 1023 1000
CommandBar mount 8178 8076 1000
ContextualMenu mount 14729 14814 1000
DefaultButton mount 1173 1167 5000
DetailsRow mount 3702 3799 5000
DetailsRowFast mount 3705 3773 5000
DetailsRowNoStyles mount 3467 3491 5000
Dialog mount 1544 1597 1000
DocumentCardTitle mount 1913 1921 1000
Dropdown mount 2562 2592 5000
FocusZone mount 1891 1927 5000
IconButton mount 1804 1811 5000
Label mount 357 379 5000
Link mount 502 508 5000
LinkNext mount 556 538 5000
MenuButton mount 1536 1531 5000
Nav mount 3389 3340 1000
Panel mount 1549 1573 1000
Persona mount 906 899 1000
Pivot mount 1466 1481 1000
PivotNext mount 1422 1395 1000
PrimaryButton mount 1322 1357 5000
SearchBox mount 1329 1421 5000
Slider mount 1590 1593 5000
SliderNext mount 2021 2161 5000
Spinner mount 441 462 5000
SplitButton mount 3328 3358 5000
Stack mount 522 554 5000
StackWithIntrinsicChildren mount 1980 2016 5000
StackWithTextChildren mount 5462 5227 5000
TagPicker mount 2847 2973 5000
Text mount 446 455 5000
TextField mount 1502 1473 5000
ThemeProvider mount 2957 3084 5000
ThemeProvider virtual-rerender 565 546 5000
Toggle mount 980 927 5000
ToggleNext mount 979 992 5000
button mount 120 116 5000

Perf Analysis (Fluent)

⚠️ 2 potential perf regressions detected

Potential regressions comparing to master

Scenario Current PR Ticks Baseline Ticks Ratio Regression Analysis
AccordionMinimalPerf.default 168 157 1.07:1 analysis
AttachmentMinimalPerf.default 179 167 1.07:1 analysis
Perf comparison
Status Scenario Fluent TPI Fabric TPI Ratio Iterations Ticks
🎯 Avatar.Fluent 0.45 0.51 0.88:1 2000 896
🦄 Button.Fluent 0.12 0.2 0.6:1 5000 608
🔧 Checkbox.Fluent 0.64 0.35 1.83:1 1000 638
🦄 Dialog.Fluent 0.15 0.23 0.65:1 5000 773
🔧 Dropdown.Fluent 3.43 0.47 7.3:1 1000 3433
🔧 Icon.Fluent 0.14 0.05 2.8:1 5000 719
🦄 Image.Fluent 0.07 0.11 0.64:1 5000 363
🔧 Slider.Fluent 1.69 0.41 4.12:1 1000 1693
🔧 Text.Fluent 0.08 0.02 4:1 5000 380
🦄 Tooltip.Fluent 0.1 20.28 0:1 5000 499

🔧 Needs work     🎯 On target     🦄 Amazing

Perf tests with no regressions
Scenario Current PR Ticks Baseline Ticks Ratio
FlexMinimalPerf.default 387 327 1.18:1
TableMinimalPerf.default 459 391 1.17:1
Button.Fluent 608 539 1.13:1
BoxMinimalPerf.default 385 344 1.12:1
AttachmentSlotsPerf.default 1304 1188 1.1:1
EmbedMinimalPerf.default 2106 1919 1.1:1
Text.Fluent 380 347 1.1:1
DialogMinimalPerf.default 862 792 1.09:1
HierarchicalTreeMinimalPerf.default 441 406 1.09:1
RefMinimalPerf.default 228 210 1.09:1
ChatWithPopoverPerf.default 554 514 1.08:1
TreeMinimalPerf.default 933 861 1.08:1
InputMinimalPerf.default 1134 1061 1.07:1
CheckboxMinimalPerf.default 3164 2999 1.06:1
ListCommonPerf.default 1008 948 1.06:1
SegmentMinimalPerf.default 346 327 1.06:1
CustomToolbarPrototype.default 4340 4113 1.06:1
HeaderSlotsPerf.default 853 816 1.05:1
LayoutMinimalPerf.default 434 415 1.05:1
Slider.Fluent 1693 1608 1.05:1
CardMinimalPerf.default 641 619 1.04:1
DropdownMinimalPerf.default 3722 3584 1.04:1
HeaderMinimalPerf.default 370 357 1.04:1
LabelMinimalPerf.default 451 435 1.04:1
ProviderMergeThemesPerf.default 2275 2186 1.04:1
ToolbarMinimalPerf.default 995 959 1.04:1
VideoMinimalPerf.default 632 607 1.04:1
ChatMinimalPerf.default 689 667 1.03:1
FormMinimalPerf.default 410 398 1.03:1
ProviderMinimalPerf.default 904 875 1.03:1
AvatarMinimalPerf.default 534 524 1.02:1
TooltipMinimalPerf.default 767 753 1.02:1
GridMinimalPerf.default 730 720 1.01:1
ListWith60ListItems.default 1154 1145 1.01:1
MenuMinimalPerf.default 852 844 1.01:1
AlertMinimalPerf.default 289 290 1:1
ListNestedPerf.default 917 919 1:1
RadioGroupMinimalPerf.default 404 406 1:1
SliderMinimalPerf.default 1661 1658 1:1
StatusMinimalPerf.default 725 728 1:1
TextAreaMinimalPerf.default 485 483 1:1
Icon.Fluent 719 721 1:1
ItemLayoutMinimalPerf.default 1415 1427 0.99:1
SplitButtonMinimalPerf.default 3507 3538 0.99:1
TableManyItemsPerf.default 2242 2276 0.99:1
Dropdown.Fluent 3433 3456 0.99:1
AnimationMinimalPerf.default 432 443 0.98:1
MenuButtonMinimalPerf.default 1274 1306 0.98:1
Tooltip.Fluent 499 509 0.98:1
LoaderMinimalPerf.default 753 774 0.97:1
TextMinimalPerf.default 335 345 0.97:1
CarouselMinimalPerf.default 508 527 0.96:1
DropdownManyItemsPerf.default 1384 1443 0.96:1
PopupMinimalPerf.default 292 304 0.96:1
PortalMinimalPerf.default 128 134 0.96:1
ButtonSlotsPerf.default 602 636 0.95:1
ButtonMinimalPerf.default 184 195 0.94:1
Image.Fluent 363 387 0.94:1
DividerMinimalPerf.default 328 358 0.92:1
ListMinimalPerf.default 478 522 0.92:1
Checkbox.Fluent 638 696 0.92:1
Dialog.Fluent 773 838 0.92:1
Avatar.Fluent 896 989 0.91:1
IconMinimalPerf.default 629 701 0.9:1
TreeWith60ListItems.default 218 242 0.9:1
ReactionMinimalPerf.default 357 400 0.89:1
ImageMinimalPerf.default 382 435 0.88:1
ChatDuplicateMessagesPerf.default 403 474 0.85:1

@size-auditor
Copy link

size-auditor bot commented Jun 11, 2020

Asset size changes

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

Baseline commit: a8748bcd3616a3a7c453f5bae0960f8efb82a19a (build)

@assuncaocharles
Copy link
Contributor Author

@jurokapsiar @layershifter @pompomon @mnajdova @miroslavstastny As agreed, in this PR Input when as the flag required will show the successIndicator the question is if we want to do it always when it's required? Previously we only showed if successIndicator were passed, but changing this will force it for all required inputs, just wanted to confirm it's what we really want.

@mnajdova
Copy link
Contributor

As agreed, in this PR Input when as the flag required will show the successIndicator the question is if we want to do it always when it's required? Previously we only showed if successIndicator were passed, but changing this will force it for all required inputs, just wanted to confirm it's what we really want.

In my opinion it would look awkward if all fields which are not required will have the success indicator.. Usually on forms, the success indicator is shown after you successfully filled the form field (which in my opinion means, that the field either was required, or required some special validation - maybe it is an email or phone number) However this is just my opinion, and we should definitely check the designs (maybe it should be shown only if the input field was focused, or the value was initialized..)

@assuncaocharles
Copy link
Contributor Author

As agreed, in this PR Input when as the flag required will show the successIndicator the question is if we want to do it always when it's required? Previously we only showed if successIndicator were passed, but changing this will force it for all required inputs, just wanted to confirm it's what we really want.

In my opinion it would look awkward if all fields which are not required will have the success indicator.. Usually on forms, the success indicator is shown after you successfully filled the form field (which in my opinion means, that the field either was required, or required some special validation - maybe it is an email or phone number) However this is just my opinion, and we should definitely check the designs (maybe it should be shown only if the input field was focused, or the value was initialized..)

But it's not what happening, the success indicator is only being shown if the field was required... The question here is because in several examples we were using required field ( Like in the input inside toolbar ) and having the indicator by default in required fields will generate regression because the input get bigger. Removing the required will generate regression because will remove * from the label...

…d.tsx

Co-authored-by: Marija Najdova <mnajdova@gmail.com>
Co-authored-by: Marija Najdova <mnajdova@gmail.com>
Copy link
Contributor

@mnajdova mnajdova 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, please add changelog entry before merging

@assuncaocharles assuncaocharles merged commit 47c3fc0 into microsoft:master Jun 12, 2020
@assuncaocharles assuncaocharles deleted the fix/form-example branch June 12, 2020 20:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Fluent UI react-northstar (v0) Work related to Fluent UI V0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants