chore(Box|Button|Image|Avatar): converting components with hooks #2238
Conversation
…stardust-ui/react into feat/use-hooks-in-box-component
…stardust-ui/react into feat/use-hooks-in-box-component
mnajdova
requested review from
dvdzkwsk,
dzearing,
ecraig12345,
JasonGore,
jdhuntington,
kenotron,
layershifter,
levithomason and
miroslavstastny
as code owners
Perf comparison
Generated by 🚫 dangerJS |
| import * as customPropTypes from '@fluentui/react-proptypes' | ||
| import * as PropTypes from 'prop-types' | ||
| import * as React from 'react' | ||
| // @ts-ignore | ||
| import { ThemeContext } from 'react-fela' |
There was a problem hiding this comment.
Avatar.tsx should not refer to fela specifics directly. @jdhuntington for heads up here.
We should expect the base component to simply take in a classes prop. So, Avatar renders DOM and injects the right classes.
Then createComponent can abstract how classes gets injected.
export const Avatar = createComponent(AvatarBase, { stuff to build `classes` attribute });
There was a problem hiding this comment.
It's not Fela specifics. We use this context only by historical reasons. We should use own context instead.
| 'larger', | ||
| 'largest', | ||
| ]) | ||
| export const accessibility = PropTypes.func |
There was a problem hiding this comment.
| // Components | ||
| // ======================================================== | ||
|
|
||
| export type FluentComponentStaticProps<P = {}> = { |
There was a problem hiding this comment.
I don't have better naming ideas
# Conflicts: # packages/react-bindings/src/hooks/useStyles.ts # packages/react/src/types.ts
layershifter
changed the title
| } | ||
|
|
||
| setEnd() |
There was a problem hiding this comment.
This is extremely fragile and (currently) wrong.
What if I forget to add telemetry to my component? There should be a conformance test for that.
But the main problem I see is that nothing prevents you from calling setStart too late or setEnd too early.
In this case the whole return is down outside of the telemetry scope - all calls to getAttributes(), getA11Props(), renderLoader(), renderIcon()... - which makes the numbers useless :-(
So we should either give up and remove fluent-ui telemetry (no!) or rethink and rework how we measure it (something like a wrapping function for the whole render? And named something like renderComponent?)
There was a problem hiding this comment.
No renderComponent please, we are trying to move out from it. This is an internal thing and we can track it (like you can block the PR because it's not in the Avatar, and Image components :D - oops will ad it) Even when this was in renderComponent, it was setting end even before invoking render() which may be even worse.. Anyway, I will add the telemetry to the other components before merging. Do you have some other proposal for the setting of the start and end?
There was a problem hiding this comment.
Even when this was in renderComponent, it was setting end even before invoking render() which may be even worse..
That's not true. That's what @layershifter broke in #1980
There was a problem hiding this comment.
Fixed renderComponent, fixed components' telemetry to be accurate. It is used internally now, so we can make sure we are adding it properly until we have better idea of how to improve it.
This PR converts
Box,Button,Image,Avatarcomponents to use our newly shipped React hooks as part of #2269.Adds
FluentComponentStaticPropstype to use Fluent specific static props (className,handledProps) on function components.BREAKING CHANGES
We are now restricting the set of props provided to the styles. Here are the props propagated to the updated components;
text,primary,disabled,circular,size,loading,inverted,iconOnly,fluidandhasContentsizeavatar,circular,fluid