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

Very basic Input implementation #19079

Merged
merged 2 commits into from
Jul 23, 2021
Merged

Very basic Input implementation #19079

merged 2 commits into from
Jul 23, 2021

Conversation

ecraig12345
Copy link
Member

@ecraig12345 ecraig12345 commented Jul 22, 2021 •
edited

Pull request checklist

Description of changes

Very basic implementation of Input. It has all the slots (with tentative names) but no styling or behavior, and none of the planned additional props.

(Note that this does NOT incorporate any of the alternative approaches for handling native props proposed in #18804 or #18983.)

@ecraig12345 ecraig12345 requested review from bsunderhus and a team July 22, 2021 21:44
@ecraig12345 ecraig12345 added this to In progress in CXE Redmond - @microsoft/cxe-red via automation Jul 22, 2021
@fabricteam
Copy link
Collaborator

fabricteam commented Jul 22, 2021 •
edited

📊 Bundle size report

🤖 This report was generated against fc6cc7d73c8d61587e26cd19b3bc9db3abd077a6

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jul 22, 2021 •
edited

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 7946c9b:

Sandbox Source
Fluent UI React Starter Configuration

@size-auditor
Copy link

size-auditor bot commented Jul 22, 2021 •
edited

Asset size changes

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

Baseline commit: fc6cc7d73c8d61587e26cd19b3bc9db3abd077a6 (build)

@fabricteam
Copy link
Collaborator

fabricteam commented Jul 22, 2021 •
edited

Perf Analysis (@fluentui/react)

No significant results to display.

All results

Scenario Render type Master Ticks PR Ticks Iterations Status
Avatar mount 773 778 5000
BaseButton mount 855 866 5000
Breadcrumb mount 2491 2501 1000
ButtonNext mount 497 494 5000
Checkbox mount 1433 1459 5000
CheckboxBase mount 1202 1211 5000
ChoiceGroup mount 4514 4575 5000
ComboBox mount 926 942 1000
CommandBar mount 9756 9639 1000
ContextualMenu mount 5982 5913 1000
DefaultButton mount 1075 1066 5000
DetailsRow mount 3623 3515 5000
DetailsRowFast mount 3580 3559 5000
DetailsRowNoStyles mount 3440 3507 5000
Dialog mount 2143 2119 1000
DocumentCardTitle mount 132 154 1000
Dropdown mount 3211 3135 5000
FluentProviderNext mount 7085 7110 5000
FocusTrapZone mount 1732 1698 5000
FocusZone mount 1736 1722 5000
IconButton mount 1650 1666 5000
Label mount 343 330 5000
Layer mount 1698 1728 5000
Link mount 460 454 5000
MakeStyles mount 1765 1738 50000
MenuButton mount 1382 1382 5000
MessageBar mount 1945 1972 5000
Nav mount 3155 3072 1000
OverflowSet mount 991 999 5000
Panel mount 1970 1966 1000
Persona mount 789 788 1000
Pivot mount 1356 1335 1000
PrimaryButton mount 1212 1201 5000
Rating mount 7226 7365 5000
SearchBox mount 1244 1248 5000
Shimmer mount 2407 2420 5000
Slider mount 1870 1850 5000
SpinButton mount 4833 4723 5000
Spinner mount 408 402 5000
SplitButton mount 2982 3015 5000
Stack mount 485 474 5000
StackWithIntrinsicChildren mount 1439 1432 5000
StackWithTextChildren mount 4236 4253 5000
SwatchColorPicker mount 9859 9607 5000
Tabs mount 1353 1363 1000
TagPicker mount 2331 2364 5000
TeachingBubble mount 11464 11450 5000
Text mount 399 393 5000
TextField mount 1291 1293 5000
ThemeProvider mount 1141 1111 5000
ThemeProvider virtual-rerender 597 576 5000
Toggle mount 769 768 5000
buttonNative mount 104 111 5000

Perf Analysis (@fluentui/react-northstar)

Perf tests with no regressions
Scenario Current PR Ticks Baseline Ticks Ratio
AnimationMinimalPerf.default 423 377 1.12:1
PortalMinimalPerf.default 174 155 1.12:1
ButtonMinimalPerf.default 165 155 1.06:1
DropdownManyItemsPerf.default 669 631 1.06:1
ImageMinimalPerf.default 362 340 1.06:1
ButtonSlotsPerf.default 526 502 1.05:1
FormMinimalPerf.default 386 369 1.05:1
LoaderMinimalPerf.default 670 642 1.04:1
BoxMinimalPerf.default 329 319 1.03:1
HeaderMinimalPerf.default 338 329 1.03:1
ListMinimalPerf.default 482 470 1.03:1
PopupMinimalPerf.default 571 556 1.03:1
TreeWith60ListItems.default 160 156 1.03:1
AttachmentSlotsPerf.default 1015 997 1.02:1
LabelMinimalPerf.default 369 363 1.02:1
LayoutMinimalPerf.default 345 337 1.02:1
MenuMinimalPerf.default 796 778 1.02:1
SkeletonMinimalPerf.default 338 332 1.02:1
TableMinimalPerf.default 381 374 1.02:1
ChatDuplicateMessagesPerf.default 274 271 1.01:1
CheckboxMinimalPerf.default 2597 2581 1.01:1
EmbedMinimalPerf.default 3908 3868 1.01:1
FlexMinimalPerf.default 268 266 1.01:1
InputMinimalPerf.default 1197 1182 1.01:1
MenuButtonMinimalPerf.default 1549 1535 1.01:1
ReactionMinimalPerf.default 345 341 1.01:1
SplitButtonMinimalPerf.default 3646 3617 1.01:1
IconMinimalPerf.default 585 580 1.01:1
TableManyItemsPerf.default 1780 1760 1.01:1
TextAreaMinimalPerf.default 464 458 1.01:1
CardMinimalPerf.default 505 503 1:1
CarouselMinimalPerf.default 426 425 1:1
ChatWithPopoverPerf.default 332 332 1:1
DividerMinimalPerf.default 341 342 1:1
DropdownMinimalPerf.default 2958 2955 1:1
GridMinimalPerf.default 314 315 1:1
ListWith60ListItems.default 602 603 1:1
ProviderMergeThemesPerf.default 1591 1592 1:1
RadioGroupMinimalPerf.default 405 407 1:1
SegmentMinimalPerf.default 317 316 1:1
TooltipMinimalPerf.default 961 958 1:1
TreeMinimalPerf.default 746 745 1:1
VideoMinimalPerf.default 575 575 1:1
ButtonOverridesMissPerf.default 1591 1615 0.99:1
ChatMinimalPerf.default 597 604 0.99:1
DatepickerMinimalPerf.default 5114 5190 0.99:1
DialogMinimalPerf.default 709 713 0.99:1
HeaderSlotsPerf.default 700 709 0.99:1
ItemLayoutMinimalPerf.default 1130 1136 0.99:1
ListCommonPerf.default 584 591 0.99:1
ListNestedPerf.default 503 510 0.99:1
StatusMinimalPerf.default 636 643 0.99:1
ToolbarMinimalPerf.default 876 881 0.99:1
AccordionMinimalPerf.default 143 146 0.98:1
RefMinimalPerf.default 217 222 0.98:1
TextMinimalPerf.default 316 324 0.98:1
CustomToolbarPrototype.default 3594 3662 0.98:1
AttachmentMinimalPerf.default 147 151 0.97:1
AvatarMinimalPerf.default 170 176 0.97:1
ProviderMinimalPerf.default 907 932 0.97:1
RosterPerf.default 1090 1133 0.96:1
SliderMinimalPerf.default 1468 1523 0.96:1
AlertMinimalPerf.default 246 274 0.9:1

CXE Redmond - @microsoft/cxe-red automation moved this from In progress to Reviewer approved Jul 23, 2021
@ecraig12345 @khmakoto
Update packages/react-input/src/components/Input/useInput.ts
7946c9b 
Co-authored-by: Makoto Morimoto <Humberto.Morimoto@microsoft.com>
behowell
behowell approved these changes Jul 23, 2021
View reviewed changes
packages/react-input/src/components/Input/Input.types.ts
*/
export type InputDefaultedProps = never; // TODO add names of properties with default values
export interface InputProps extends ComponentProps<Partial<InputSlots>>, Partial<InputCommons> {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably shouldn't add Partial around InputCommons, All HTMLAttributes are optional, and any other props you add later should be optional if they are indeed optional, rather than using Partial here.

Suggested change
export interface InputProps extends ComponentProps<Partial<InputSlots>>, Partial<InputCommons> {}
export interface InputProps extends ComponentProps<Partial<InputSlots>>, InputCommons {}

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure that's true. I could see a case where I'd want to ensure a particular prop was always present in state (with a default) but could be optional in props. And I'm pretty sure all of Input's props will be optional.

packages/react-input/src/components/Input/renderInput.tsx
Comment on lines +11 to +12
delete slotProps.input.children;
delete slotProps.inputWrapper.children;
Copy link
Contributor

Choose a reason for hiding this comment

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

These deletes seem a little fishy. You should throw an error if you find unsupported children instead. Or even better, fix the typings if children are not supported (Omit 'children')

Copy link
Member Author

Choose a reason for hiding this comment

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

Right now the slots would get null rendered if they didn't have children (so I have to pass fake children in useInput). @bsunderhus has an RFC about how to handle that better.

Copy link
Contributor

Choose a reason for hiding this comment

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

An RFC #18949 that would love more reviews 😉 @behowell

@ecraig12345 ecraig12345 merged commit 2626e3c into microsoft:master Jul 23, 2021
CXE Redmond - @microsoft/cxe-red automation moved this from Reviewer approved to Done Jul 23, 2021
PeterDraex pushed a commit to PeterDraex/fluentui that referenced this pull request Aug 6, 2021
@ecraig12345 @PeterDraex
Very basic Input implementation (microsoft#19079)
f2a88a3
@ecraig12345 ecraig12345 deleted the basic-input branch September 27, 2021 04:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bsunderhus bsunderhus bsunderhus approved these changes

@khmakoto khmakoto khmakoto approved these changes

@ling1726 ling1726 ling1726 approved these changes

@behowell behowell behowell approved these changes

Assignees

@ling1726 ling1726

Labels
Component: Input
Projects
No open projects
CXE Redmond - @microsoft/cxe-red
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@ecraig12345 @fabricteam @bsunderhus @khmakoto @ling1726 @behowell