Input: Add styling #19376
Input: Add styling #19376
Conversation
📊 Bundle size report
🤖 This report was generated against cfddfa922c3931371abb7c0e047783e5b49b33c0 |
|
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 b4041ae:
|
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: cfddfa922c3931371abb7c0e047783e5b49b33c0 (build) |
Perf Analysis (
|
| Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
|---|---|---|---|---|---|
| Avatar | mount | 970 | 962 | 5000 | |
| BaseButton | mount | 967 | 995 | 5000 | |
| Breadcrumb | mount | 2597 | 2571 | 1000 | |
| ButtonNext | mount | 461 | 466 | 5000 | |
| Checkbox | mount | 1567 | 1623 | 5000 | |
| CheckboxBase | mount | 1381 | 1372 | 5000 | |
| ChoiceGroup | mount | 5012 | 5069 | 5000 | |
| ComboBox | mount | 1080 | 1017 | 1000 | |
| CommandBar | mount | 10318 | 10215 | 1000 | |
| ContextualMenu | mount | 6247 | 6385 | 1000 | |
| DefaultButton | mount | 1196 | 1215 | 5000 | |
| DetailsRow | mount | 3826 | 4002 | 5000 | |
| DetailsRowFast | mount | 3957 | 3873 | 5000 | |
| DetailsRowNoStyles | mount | 3622 | 3680 | 5000 | |
| Dialog | mount | 2226 | 2194 | 1000 | |
| DocumentCardTitle | mount | 165 | 153 | 1000 | |
| Dropdown | mount | 3430 | 3376 | 5000 | |
| FluentProviderNext | mount | 7065 | 7095 | 5000 | |
| FocusTrapZone | mount | 1855 | 1824 | 5000 | |
| FocusZone | mount | 1788 | 1828 | 5000 | |
| IconButton | mount | 1874 | 1891 | 5000 | |
| Label | mount | 346 | 332 | 5000 | |
| Layer | mount | 1848 | 1876 | 5000 | |
| Link | mount | 484 | 469 | 5000 | |
| MakeStyles | mount | 1803 | 1778 | 50000 | |
| MenuButton | mount | 1556 | 1558 | 5000 | |
| MessageBar | mount | 2023 | 2031 | 5000 | |
| Nav | mount | 3501 | 3423 | 1000 | |
| OverflowSet | mount | 1123 | 1172 | 5000 | |
| Panel | mount | 2124 | 2114 | 1000 | |
| Persona | mount | 866 | 867 | 1000 | |
| Pivot | mount | 1468 | 1417 | 1000 | |
| PrimaryButton | mount | 1334 | 1380 | 5000 | |
| Rating | mount | 8352 | 8125 | 5000 | |
| SearchBox | mount | 1403 | 1413 | 5000 | |
| Shimmer | mount | 2875 | 2686 | 5000 | |
| Slider | mount | 2068 | 2033 | 5000 | |
| SpinButton | mount | 5352 | 5266 | 5000 | |
| Spinner | mount | 426 | 427 | 5000 | |
| SplitButton | mount | 3275 | 3251 | 5000 | |
| Stack | mount | 508 | 529 | 5000 | |
| StackWithIntrinsicChildren | mount | 1659 | 1686 | 5000 | |
| StackWithTextChildren | mount | 4971 | 4939 | 5000 | |
| SwatchColorPicker | mount | 10774 | 10736 | 5000 | |
| Tabs | mount | 1470 | 1476 | 1000 | |
| TagPicker | mount | 2726 | 2784 | 5000 | |
| TeachingBubble | mount | 11920 | 12052 | 5000 | |
| Text | mount | 449 | 440 | 5000 | |
| TextField | mount | 1458 | 1440 | 5000 | |
| ThemeProvider | mount | 1247 | 1209 | 5000 | |
| ThemeProvider | virtual-rerender | 603 | 587 | 5000 | |
| Toggle | mount | 850 | 848 | 5000 | |
| buttonNative | mount | 116 | 105 | 5000 |
Perf Analysis (@fluentui/react-northstar)
Perf tests with no regressions
| Scenario | Current PR Ticks | Baseline Ticks | Ratio |
|---|---|---|---|
| TreeWith60ListItems.default | 200 | 167 | 1.2:1 |
| FlexMinimalPerf.default | 310 | 266 | 1.17:1 |
| ReactionMinimalPerf.default | 423 | 383 | 1.1:1 |
| PortalMinimalPerf.default | 178 | 163 | 1.09:1 |
| ImageMinimalPerf.default | 409 | 381 | 1.07:1 |
| AttachmentMinimalPerf.default | 164 | 155 | 1.06:1 |
| LayoutMinimalPerf.default | 402 | 378 | 1.06:1 |
| MenuMinimalPerf.default | 898 | 851 | 1.06:1 |
| TextMinimalPerf.default | 369 | 347 | 1.06:1 |
| AnimationMinimalPerf.default | 437 | 418 | 1.05:1 |
| CarouselMinimalPerf.default | 492 | 469 | 1.05:1 |
| ChatMinimalPerf.default | 705 | 669 | 1.05:1 |
| ListMinimalPerf.default | 562 | 536 | 1.05:1 |
| ListNestedPerf.default | 605 | 575 | 1.05:1 |
| RefMinimalPerf.default | 234 | 222 | 1.05:1 |
| SkeletonMinimalPerf.default | 385 | 365 | 1.05:1 |
| ChatDuplicateMessagesPerf.default | 315 | 304 | 1.04:1 |
| DividerMinimalPerf.default | 375 | 362 | 1.04:1 |
| TextAreaMinimalPerf.default | 538 | 518 | 1.04:1 |
| TreeMinimalPerf.default | 851 | 822 | 1.04:1 |
| ButtonMinimalPerf.default | 185 | 179 | 1.03:1 |
| HeaderMinimalPerf.default | 381 | 370 | 1.03:1 |
| ToolbarMinimalPerf.default | 1004 | 972 | 1.03:1 |
| TooltipMinimalPerf.default | 1072 | 1041 | 1.03:1 |
| RadioGroupMinimalPerf.default | 478 | 468 | 1.02:1 |
| ChatWithPopoverPerf.default | 381 | 376 | 1.01:1 |
| DropdownManyItemsPerf.default | 722 | 716 | 1.01:1 |
| DropdownMinimalPerf.default | 3107 | 3080 | 1.01:1 |
| GridMinimalPerf.default | 374 | 372 | 1.01:1 |
| HeaderSlotsPerf.default | 810 | 804 | 1.01:1 |
| ItemLayoutMinimalPerf.default | 1272 | 1259 | 1.01:1 |
| LoaderMinimalPerf.default | 718 | 713 | 1.01:1 |
| MenuButtonMinimalPerf.default | 1711 | 1690 | 1.01:1 |
| StatusMinimalPerf.default | 726 | 716 | 1.01:1 |
| CustomToolbarPrototype.default | 3945 | 3916 | 1.01:1 |
| ButtonSlotsPerf.default | 569 | 569 | 1:1 |
| DatepickerMinimalPerf.default | 5513 | 5497 | 1:1 |
| DialogMinimalPerf.default | 800 | 798 | 1:1 |
| FormMinimalPerf.default | 433 | 435 | 1:1 |
| InputMinimalPerf.default | 1291 | 1297 | 1:1 |
| ListWith60ListItems.default | 674 | 672 | 1:1 |
| PopupMinimalPerf.default | 605 | 603 | 1:1 |
| ProviderMinimalPerf.default | 1043 | 1042 | 1:1 |
| SplitButtonMinimalPerf.default | 4373 | 4360 | 1:1 |
| TableMinimalPerf.default | 424 | 422 | 1:1 |
| VideoMinimalPerf.default | 650 | 652 | 1:1 |
| CheckboxMinimalPerf.default | 2792 | 2812 | 0.99:1 |
| EmbedMinimalPerf.default | 4251 | 4295 | 0.99:1 |
| ProviderMergeThemesPerf.default | 1634 | 1645 | 0.99:1 |
| SegmentMinimalPerf.default | 359 | 362 | 0.99:1 |
| AttachmentSlotsPerf.default | 1099 | 1122 | 0.98:1 |
| AvatarMinimalPerf.default | 196 | 201 | 0.98:1 |
| ButtonOverridesMissPerf.default | 1709 | 1748 | 0.98:1 |
| IconMinimalPerf.default | 642 | 653 | 0.98:1 |
| TableManyItemsPerf.default | 1989 | 2026 | 0.98:1 |
| LabelMinimalPerf.default | 395 | 407 | 0.97:1 |
| RosterPerf.default | 1261 | 1302 | 0.97:1 |
| SliderMinimalPerf.default | 1622 | 1686 | 0.96:1 |
| AlertMinimalPerf.default | 279 | 295 | 0.95:1 |
| ListCommonPerf.default | 652 | 692 | 0.94:1 |
| CardMinimalPerf.default | 574 | 617 | 0.93:1 |
| BoxMinimalPerf.default | 342 | 375 | 0.91:1 |
| AccordionMinimalPerf.default | 150 | 167 | 0.9:1 |
ecraig12345
force-pushed
the
input-styling
branch
2 times, most recently
from
400d3c2
to
19e0f00
Compare
ecraig12345
force-pushed
the
input-styling
branch
3 times, most recently
from
c58fce5
to
596ddf2
Compare
ecraig12345
force-pushed
the
input-styling
branch
from
596ddf2
to
285a9cd
Compare
ecraig12345
force-pushed
the
input-styling
branch
from
285a9cd
to
3efbb1e
Compare
ecraig12345
force-pushed
the
input-styling
branch
from
920550c
to
9281935
Compare
| export const Input: React_2.FunctionComponent<InputProps>; | ||
|
|
||
| // @public (undocumented) | ||
| export interface InputCommons { |
There was a problem hiding this comment.
Nit: InputCommonProps? or CommonInputProps?
There was a problem hiding this comment.
I was copying the ___Commons convention from a few of the other converged components, for things shared between props and state. So I'd probably prefer to keep it as-is, but I don't feel super strongly about it.
| /** | ||
| * Input Props | ||
| */ | ||
| export interface InputProps extends ComponentProps<Partial<InputSlots>> {} |
There was a problem hiding this comment.
Question - when moving to React hooks, many teams choose to move from interface to type declarations for their props and use type intersections over extends. Are interfaces giving us something that types would not? When I see interfaces I always think a class will implement it.
There was a problem hiding this comment.
IIRC we discussed this but didn't come to a solid conclusion about which convention we wanted (and the discussion apparently happened before we started documenting things in RFCs). There may be some specific reason that type is commonly used for slots, but I can't remember offhand. @layershifter do you remember?
There was a problem hiding this comment.
There may be some specific reason that
typeis commonly used for slots, but I can't remember offhand. @layershifter do you remember?
When TS emits type declarations sometimes it inlines type declarations (microsoft/TypeScript#37151), so in general interfaces are preferred for in case.
We have seen extreme cases where this duplication has inflated declaration files from 7KB to 700KB. That’s quite a lot of redundant code to download and parse.
Through experimentation, we discovered potential techniques to prevent inlining of type declarations, such as:
- prefer
interfaceinstead oftype(interfaces are not inlined)- If an interface needed by a declaration is not exported, tsc will refuse to inline the type and will generate a clear error (e.g.,
TS4023: Exported variable has or is using name from external module but cannot be named.)
https://www.techatbloomberg.com/blog/10-insights-adopting-typescript-at-scale/
There was a problem hiding this comment.
Interesting, thanks! Incidentally this diff (the old version) shows an example of expanded types. 😬 https://github.com/microsoft/fluentui/pull/19376/files#diff-805da48a70295edd847bed2a0dda92910c21f9f0185fa238bc342d167ae4407b
| import { getNativeElementProps, useId } from '@fluentui/react-utilities'; | ||
| import { InputProps } from './Input.types'; | ||
| import { ArgTypes } from '@storybook/react'; | ||
| // prevent terrible reload times by using deep imports :( |
| @@ -2,12 +2,16 @@ | |||
|
|
|||
| exports[`Input renders a default state 1`] = ` | |||
| <div> | |||
| <div | |||
| <span | |||
There was a problem hiding this comment.
Is it expected to have spans there? Why not divs?
There was a problem hiding this comment.
It's intentional (there's a comment about this in renderInput). If the input is inline within a p there will be nesting errors if I use div.
In a previous version of the implementation I used div by default and span if inline was true, but someone suggested just using span always since it (probably?) doesn't make much practical difference (all the elements either have display explicitly set, or are within a flex layout). I'm open to suggestions here though, and could go back to that if there's some reason it would be better.
| m: '12px', | ||
| }; | ||
| const contentSizes = { | ||
| // TODO(sharing) shouldn't these be in the theme? |
There was a problem hiding this comment.
Is it the same as react-text/src/typographyStyles/typographyStyles.ts? Should we reuse typographyStyles?
There was a problem hiding this comment.
It's similar but doesn't quite cover all the cases--the design for Input specifies large as using 400 size but I'm pretty sure it still uses regular font weight. Though it's possible that was an oversight. (Incidentally...WHY did we have to use numeric size names that look like font weights...?? I know this is more of a complaint for design and probably too late. 😞)
There was a problem hiding this comment.
Also the individual typography styles actually aren't exported at the moment. Should we change that (@andrefcdias)? In this case it's not feasible/desirable to use the full text components like Body. (Either way, since it's a change in a separate package I think I'll leave this as-is for this PR.)
| @@ -7,7 +7,7 @@ import type { InputProps } from './Input.types'; | |||
| /** | |||
| * Input component | |||
| */ | |||
| export const Input = React.forwardRef<HTMLElement, InputProps>((props, ref) => { | |||
| export const Input: React.FunctionComponent<InputProps> = React.forwardRef((props, ref) => { | |||
There was a problem hiding this comment.
I don't think that this typing is correct as React.FunctionComponent does not have ref on it's interface:
Type '{ foo: string; ref: RefObject<unknown>; }' is not assignable to type 'IntrinsicAttributes & Props & { children?: ReactNode; }'.
Property 'ref' does not exist on type 'IntrinsicAttributes & Props & { children?: ReactNode; }'.
There was a problem hiding this comment.
InputProps has ref through inheritance. I strongly prefer an explicit type for the component here because it makes the public API cleaner.
| </div> | ||
| <Input {...props} insideStart={<SearchIcon />} insideEnd={<DismissIcon />} /> |
There was a problem hiding this comment.
Not necessarily something to change now and in this PR, but in the future I think all our storybook examples should be accessible by default (i.e. have a visible label and no placeholder), with potentially one example of less-accessible variations like not having a visible label.
There was a problem hiding this comment.
Yup I plan to follow up on this when I make a more finished version of the stories
ecraig12345
force-pushed
the
input-styling
branch
from
a88a410
to
402db57
Compare
|
Going to merge this once the build passes and follow up on open discussions in later PRs. |
Pull request checklist
Description of changes
Initial implementation of Input styling. Prop naming is tentative subject to review.
Note that this DOES NOT handle hover styling, focus styling, or bookends.
Live demo here!
I currently have a single story with a bunch of knobs to try different states. This will be updated to separate stories later. I also added a temporary entry on the PR deploy site for Input (will be removed once it's exported from react-components).