react-card - Phase 1 #19646
react-card - Phase 1 #19646
Conversation
📊 Bundle size reportUnchanged fixtures
|
|
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 326b919:
|
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: 9cee1b15de24665a3f4f70f8cc450fd7d06d36e8 (build) |
| }, | ||
| }, | ||
| }); | ||
| const LogoBackground = (props: React.HTMLAttributes<HTMLElement>) => { |
There was a problem hiding this comment.
Sample component too look like the design specs
|
|
||
| // Size: medium | ||
| // TODO: Validate if we should use a token instead + the unit of said token | ||
| // TODO: Explore alternate way of applying padding |
There was a problem hiding this comment.
At the moment, we're applying padding on the Card component instead of each child component for ease of implementation reasons. However, this means the users will need to revert this padding if they so have such a specific need, so perhaps having a spacing token and reusing this across all Card* components is a better way to go.
| (state.onClick || | ||
| state.onMouseUp || | ||
| state.onMouseDown || | ||
| state.onPointerUp || | ||
| state.onPointerDown || | ||
| state.onTouchStart || | ||
| state.onTouchEnd) && |
There was a problem hiding this comment.
To be extracted into a separate utility once we define our standard of interactivity.
There was a problem hiding this comment.
I would avoid making styling assumptions based on event listeners used as it might lead to higher amount of custom overrides. Having explicit prop like interactive?: boolean is more robust. You can consider changing the default value of it based on the listeners (like if onClick is used and interactive is not, it would still be interactive, unless consumer specifies interactive={false})
There was a problem hiding this comment.
I don't see what the point of an interactive prop would be if the styling is still applied based on events. To me this is a visual accessibility feature that we want to be there.
If the user wants actions with no visual feedback or vice-versa, this seems like behavior we wouldn't want to encourage.
There was a problem hiding this comment.
@jurokapsiar That's a valid point and even more of a reason to have this logic abstracted into a separate utility which can reason about it internally based on state.
I am, however, a bit afraid of a prop like interactive?: boolean as it exposes styling internals. This will also mean we run into a situation where we decouple styling interactivity and functional interactivity.
- What should happen if the prop is true but there are no handlers?
- Should it look interactive but when users try to manipulate it, it does nothing?
- What should happen if the prop is false and there are handlers?
- Should it disable those handlers?
- Should it look non-interactive? But how are we then handling cases like keyboard focus?
If we believe that current component spec will lead to high amount of custom overrides then in my opinion the component design is not done properly and interactivity should be better defined within the spec. Otherwise, this would just be a premature optimisation for a problem that might not be a problem at all, especially when taking into consideration there already is a mechanism for users to override the styling if they want to opt-into less common scenarios.
There was a problem hiding this comment.
Looking at the figma design specs, there is no requirement that these interactive styles are linked to any kind of event listeners.
Why does interactive necessarily mean a mouse click ? why can a mouseenter not define interactive behaviour ? If a user wants to have hover styles, they would currently need to add an empty click listener to do that.
The event listeners also miss out on contextmenu event, should that also define interactivity ?
Adding an interactive prop means that we allow maximum flexibility for users since the designers who will work with these components can be from different product groups and might not associate things like hover styles with a click. On the reverse, some designs might not want hover styles or pressed styles to be applied on the card even on click, the simple prop lets us quite easily handle both situations without being too hacky
There was a problem hiding this comment.
@andrefcdias @jurokapsiar @ling1726 I opened an issue which covers this. I feel this discussion targets more than just the card and feel that extra visibility of a separate issue might be useful. If you could raise your points there it would be greatly welcome. 🙏
There was a problem hiding this comment.
This is why we need to define interactivity. My personal opinion is that if something has an action, it's interactive and needs visual feedback of being actionable. A mouseenter event is not an event that, to me, would define interactivity.
End of the day, this is something that should be opt out. If the user doesn't define anything, we provide them with a good baseline.
My suggestion is that we go with the current solution, keeping in mind that this is definitely not set in stone. There's already the pretext that breaking changes will happen in the next phases, so let's not dwell on this right now and migrate this discussion into the spec PR to define the definite solution for it, unless this is of high importance to have defined right now.
What do you think?
packages/react-card/src/components/CardHeader/renderCardHeader.tsx
Outdated
Show resolved
Hide resolved
Perf Analysis (
|
| Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
|---|---|---|---|---|---|
| Avatar | mount | 936 | 900 | 5000 | |
| BaseButton | mount | 892 | 953 | 5000 | |
| Breadcrumb | mount | 2587 | 2632 | 1000 | |
| ButtonNext | mount | 487 | 443 | 5000 | |
| Checkbox | mount | 1497 | 1511 | 5000 | |
| CheckboxBase | mount | 1334 | 1317 | 5000 | |
| ChoiceGroup | mount | 4730 | 4772 | 5000 | |
| ComboBox | mount | 965 | 973 | 1000 | |
| CommandBar | mount | 10276 | 10318 | 1000 | |
| ContextualMenu | mount | 6639 | 6533 | 1000 | |
| DefaultButton | mount | 1085 | 1128 | 5000 | |
| DetailsRow | mount | 3778 | 3721 | 5000 | |
| DetailsRowFast | mount | 3616 | 3767 | 5000 | |
| DetailsRowNoStyles | mount | 3512 | 3494 | 5000 | |
| Dialog | mount | 2446 | 2386 | 1000 | |
| DocumentCardTitle | mount | 125 | 138 | 1000 | |
| Dropdown | mount | 3313 | 3259 | 5000 | |
| FluentProviderNext | mount | 7470 | 7529 | 5000 | |
| FluentProviderWithTheme | mount | 344 | 352 | 10 | |
| FluentProviderWithTheme | virtual-rerender | 90 | 94 | 10 | |
| FluentProviderWithTheme | virtual-rerender-with-unmount | 491 | 496 | 10 | |
| FocusTrapZone | mount | 1767 | 1742 | 5000 | |
| FocusZone | mount | 1847 | 1835 | 5000 | |
| IconButton | mount | 1805 | 1897 | 5000 | |
| Label | mount | 332 | 340 | 5000 | |
| Layer | mount | 2933 | 2907 | 5000 | |
| Link | mount | 474 | 455 | 5000 | |
| MakeStyles | mount | 1867 | 1826 | 50000 | |
| MenuButton | mount | 1438 | 1497 | 5000 | |
| MessageBar | mount | 2058 | 1977 | 5000 | |
| Nav | mount | 3345 | 3263 | 1000 | |
| OverflowSet | mount | 1090 | 1080 | 5000 | |
| Panel | mount | 2396 | 2305 | 1000 | |
| Persona | mount | 840 | 828 | 1000 | |
| Pivot | mount | 1405 | 1426 | 1000 | |
| PrimaryButton | mount | 1301 | 1249 | 5000 | |
| Rating | mount | 7601 | 7459 | 5000 | |
| SearchBox | mount | 1311 | 1317 | 5000 | |
| Shimmer | mount | 2502 | 2506 | 5000 | |
| Slider | mount | 1960 | 2001 | 5000 | |
| SpinButton | mount | 4950 | 5277 | 5000 | |
| Spinner | mount | 441 | 415 | 5000 | |
| SplitButton | mount | 3249 | 3137 | 5000 | |
| Stack | mount | 512 | 537 | 5000 | |
| StackWithIntrinsicChildren | mount | 1549 | 1551 | 5000 | |
| StackWithTextChildren | mount | 4492 | 4472 | 5000 | |
| SwatchColorPicker | mount | 10234 | 10296 | 5000 | |
| Tabs | mount | 1380 | 1397 | 1000 | |
| TagPicker | mount | 2571 | 2529 | 5000 | |
| TeachingBubble | mount | 13301 | 13323 | 5000 | |
| Text | mount | 424 | 426 | 5000 | |
| TextField | mount | 1401 | 1368 | 5000 | |
| ThemeProvider | mount | 1196 | 1200 | 5000 | |
| ThemeProvider | virtual-rerender | 601 | 595 | 5000 | |
| ThemeProvider | virtual-rerender-with-unmount | 1866 | 1878 | 5000 | |
| Toggle | mount | 837 | 851 | 5000 | |
| buttonNative | mount | 114 | 111 | 5000 |
Perf Analysis (@fluentui/react-northstar)
Perf tests with no regressions
| Scenario | Current PR Ticks | Baseline Ticks | Ratio |
|---|---|---|---|
| AccordionMinimalPerf.default | 161 | 149 | 1.08:1 |
| AttachmentMinimalPerf.default | 162 | 150 | 1.08:1 |
| LoaderMinimalPerf.default | 732 | 679 | 1.08:1 |
| ButtonMinimalPerf.default | 173 | 161 | 1.07:1 |
| DropdownManyItemsPerf.default | 687 | 640 | 1.07:1 |
| FormMinimalPerf.default | 403 | 376 | 1.07:1 |
| ReactionMinimalPerf.default | 376 | 352 | 1.07:1 |
| ChatWithPopoverPerf.default | 381 | 358 | 1.06:1 |
| GridMinimalPerf.default | 342 | 324 | 1.06:1 |
| RadioGroupMinimalPerf.default | 451 | 426 | 1.06:1 |
| HeaderSlotsPerf.default | 794 | 754 | 1.05:1 |
| DialogMinimalPerf.default | 762 | 734 | 1.04:1 |
| ListCommonPerf.default | 615 | 592 | 1.04:1 |
| TreeWith60ListItems.default | 176 | 169 | 1.04:1 |
| AvatarMinimalPerf.default | 198 | 192 | 1.03:1 |
| TextMinimalPerf.default | 353 | 343 | 1.03:1 |
| AttachmentSlotsPerf.default | 1121 | 1102 | 1.02:1 |
| DividerMinimalPerf.default | 350 | 344 | 1.02:1 |
| InputMinimalPerf.default | 1264 | 1245 | 1.02:1 |
| ProviderMinimalPerf.default | 1060 | 1040 | 1.02:1 |
| SegmentMinimalPerf.default | 337 | 330 | 1.02:1 |
| BoxMinimalPerf.default | 345 | 340 | 1.01:1 |
| EmbedMinimalPerf.default | 4197 | 4139 | 1.01:1 |
| HeaderMinimalPerf.default | 359 | 356 | 1.01:1 |
| ItemLayoutMinimalPerf.default | 1190 | 1180 | 1.01:1 |
| LayoutMinimalPerf.default | 362 | 358 | 1.01:1 |
| MenuButtonMinimalPerf.default | 1615 | 1597 | 1.01:1 |
| RefMinimalPerf.default | 231 | 229 | 1.01:1 |
| TableManyItemsPerf.default | 1921 | 1904 | 1.01:1 |
| TooltipMinimalPerf.default | 1012 | 1006 | 1.01:1 |
| CarouselMinimalPerf.default | 451 | 450 | 1:1 |
| ChatMinimalPerf.default | 642 | 642 | 1:1 |
| CheckboxMinimalPerf.default | 2772 | 2767 | 1:1 |
| ListWith60ListItems.default | 645 | 643 | 1:1 |
| SliderMinimalPerf.default | 1550 | 1556 | 1:1 |
| TextAreaMinimalPerf.default | 483 | 485 | 1:1 |
| CustomToolbarPrototype.default | 3844 | 3857 | 1:1 |
| TreeMinimalPerf.default | 793 | 796 | 1:1 |
| AlertMinimalPerf.default | 259 | 261 | 0.99:1 |
| ButtonOverridesMissPerf.default | 1669 | 1682 | 0.99:1 |
| CardMinimalPerf.default | 543 | 547 | 0.99:1 |
| DatepickerMinimalPerf.default | 5521 | 5563 | 0.99:1 |
| DropdownMinimalPerf.default | 3081 | 3114 | 0.99:1 |
| FlexMinimalPerf.default | 285 | 287 | 0.99:1 |
| RosterPerf.default | 1113 | 1128 | 0.99:1 |
| SplitButtonMinimalPerf.default | 4089 | 4131 | 0.99:1 |
| StatusMinimalPerf.default | 647 | 653 | 0.99:1 |
| ToolbarMinimalPerf.default | 899 | 909 | 0.99:1 |
| AnimationMinimalPerf.default | 409 | 416 | 0.98:1 |
| MenuMinimalPerf.default | 835 | 854 | 0.98:1 |
| PopupMinimalPerf.default | 591 | 602 | 0.98:1 |
| TableMinimalPerf.default | 410 | 418 | 0.98:1 |
| ButtonSlotsPerf.default | 516 | 532 | 0.97:1 |
| ListNestedPerf.default | 528 | 544 | 0.97:1 |
| PortalMinimalPerf.default | 180 | 186 | 0.97:1 |
| SkeletonMinimalPerf.default | 349 | 359 | 0.97:1 |
| IconMinimalPerf.default | 582 | 608 | 0.96:1 |
| ChatDuplicateMessagesPerf.default | 285 | 301 | 0.95:1 |
| ImageMinimalPerf.default | 355 | 375 | 0.95:1 |
| ProviderMergeThemesPerf.default | 1686 | 1775 | 0.95:1 |
| LabelMinimalPerf.default | 363 | 385 | 0.94:1 |
| ListMinimalPerf.default | 495 | 526 | 0.94:1 |
| VideoMinimalPerf.default | 597 | 634 | 0.94:1 |
| @@ -0,0 +1,9 @@ | |||
| <svg width="22" height="23" viewBox="0 0 22 23" fill="none" xmlns="http://www.w3.org/2000/svg"> | |||
There was a problem hiding this comment.
you'll need to make sure these aren't released along with our published packages. I would suggest you update the migration generator to update the generated npmignore for either:
- assets folder -> could be a bit too opinionated and specific to one package
- all svgs -> I don't think we should be shipping any svgs, but I'm not 100% sure on this
There was a problem hiding this comment.
Should I just use @fluentui/example-data instead or is this a v8 thing only?
There was a problem hiding this comment.
I think it's a v8 thing, but I don't think including it as a dev dep should have too many consequences just like other shared dev deps between v8/9. @ecraig12345 any reason we can't use that package as a dev dep for stories ?
| import type { CardState } from './Card.types'; | ||
|
|
||
| /** | ||
| * Render the final JSX of Card | ||
| */ | ||
| export const renderCard = (state: CardState) => { | ||
| const { slots, slotProps } = getSlotsCompat(state, cardShorthandProps); | ||
| const { slots, slotProps } = getSlotsCompat(state); |
There was a problem hiding this comment.
since this is the first phase of a new component, please follow the new slot guidelines from #19483 It's a beta high priority task to migrate components to the new API
There was a problem hiding this comment.
Compat was used on purpose as the PR was still pending approval back then. If this is not breaking, I would rather leave it for when the implementation of the rest of the functionality starts to avoid delaying it (given that it's not one of the Beta components and we're not closing Card with this).
Could this be postponed to the next PR?
There was a problem hiding this comment.
I would say that when there's less functionality this change is easier to make (having had to do this for the full Menu package). But you are driving the work on card, so I'll leave it to your timeline 👍
There was a problem hiding this comment.
Plan would be to pick that up right after the PR gets merged and before we start with additional functionality, so impact should be the same.
|
|
||
| return ( | ||
| <slots.root {...slotProps.root}> | ||
| <div>{state.children}</div> |
There was a problem hiding this comment.
I also don't like this, but every DOM slot should be customizable. In MenuItem I had the same issue and just settled for a content slot
| <div>{state.children}</div> | |
| <slots.content>{state.children}</slots.content> |
There was a problem hiding this comment.
I see now that you do have a content slot, did you forget to use it in the render function ?
There was a problem hiding this comment.
It's under renderCardHeader.tsx
There was a problem hiding this comment.
so the same should for done for renderCardFooter ?
There was a problem hiding this comment.
I see the confusion now. I thought this was referring to CardHeader and not CardFooter. 😅
I think I would prefer to have slightly more complex styling to achieve the same result instead of having a content prop for this case. It's only there so we have content to the left and actions to the right.
Opinion?
There was a problem hiding this comment.
why not just use flex display and set margin-left: 'auto' on the actions slot ? I checked out your PR to test it out, it seems to work
There was a problem hiding this comment.
I went ahead with creating a content slot instead. margin-left would not work for RTL scenarios and there doesn't seem to be any viable solution for it. There's the :dir psudo-selector but that's not fully supported yet.
There was a problem hiding this comment.
I'm pretty sure sure make-styles does RTL styles flipping automatically
There was a problem hiding this comment.
I've reverted the API changes back to leverage that functionality. Let us discuss going forward over the best approach given that this depends on knowledge about the FluentProvider
| @@ -1,7 +1,7 @@ | |||
| { | |||
| "name": "@fluentui/react-card", | |||
| "version": "9.0.0-alpha.0", | |||
| "private": true, | |||
| "private": false, | |||
There was a problem hiding this comment.
are we sure that we want to release immediately ? are we in a rush to deliver this to partners ?
There was a problem hiding this comment.
as you said it's not targeted for the beta release
There was a problem hiding this comment.
Partner's needs are basically ASAP, hence the need for this first phase iteration.
There was a problem hiding this comment.
Another note: if partners consume this as a separate package along with react-components there would be compatibility issues since the removal of mergeProps and other breaking changes to slots. It would be hard for them to figure out why things are breaking because we use prerelease tags that don't communicate breaking changes
| /** | ||
| * Image slot | ||
| */ | ||
| image: ShorthandPropsCompat<React.ImgHTMLAttributes<HTMLImageElement>>; |
There was a problem hiding this comment.
Yes, at least for now. We don't really have any designs at the moment for scenarios with missing pieces and making it optional in the future wouldn't bring breaking changes.
Co-authored-by: Peter Varholak <peter.varholak@gmail.com>
Co-authored-by: Peter Varholak <peter.varholak@gmail.com>
Pull request checklist
- Addresses an existing issue: Fixes #0000- Include a change request file using$ yarn changeDescription of changes
To avoid delays with the implementation of partner needed functionalities of Card, a phase 1 was established where we tackle only the necessary functionalities needed.
This means that, in new phases after we approve the component spec, there will probably be breaking changes to the component.
Focus areas to test