New Typography component #506

vmilan · 2022-10-25T13:15:58Z

Description

[sc-265549]

Create a Typography component to extend Mui Typography so we can have custom props to avoid having these kinds of variants:

  'body1Italic',
  'body1Strong',
  'body1StrongItalic',
  'body2Italic',
  'body2Strong',
  'body2StrongItalic',
  'captionItalic',
  'captionStrong',
  'captionStrongItalic',
  'overlineDelicate',
  'code1',
  'code1Strong',
  'code2',
  'code2Strong',
  'code3',
  'code3Strong'

zbigg

I really like the idea.

I'm missing also .d.ts file, so we can have proper IDE help for using this new component.

zbigg · 2022-10-26T07:46:41Z

packages/react-ui/src/atoms/Typography/index.js

+};
+
+Typography.propTypes = {
+  weight: PropTypes.oneOf([400 | 500 | 600]),


I would propose to use same vocabulary as design i.e regular, semiBold ...

Absolutely, I'll do it.

zbigg · 2022-10-26T07:48:31Z

packages/react-ui/src/atoms/Typography/index.js

+const Typography = ({ italic = false, weight, children, ...otherProps }) => {
+  return (
+    <MuiTypography {...otherProps}>
+      <span style={{ fontWeight: weight, fontStyle: italic && 'italic' }}>


(i know it's MUI5, so not sure)

Isn't it possible to apply custom style on original element? Like <MuiTypography style={...}/> or <MuiTypography className={ourClass}/>.
I worry that additional dom element is just waste of resources.

Yes, sure... making some changes I left this unneeded span, but I already deleted it. Thank you!

zbigg · 2022-10-26T07:50:08Z

packages/react-ui/src/atoms/Typography/index.js

+import PropTypes from 'prop-types';
+import { Typography as MuiTypography } from '@mui/material';
+
+const Typography = ({ italic = false, weight, children, ...otherProps }) => {


I know it's typography PR, i expect we would have extended support for our colors, we've just got much more colors that MUI native primary, secondary, textPrimary ...

Yes, we have indeed, you're gonna be able to do that, like this:

<Typography color='default.background'> {someText} </Typography>

zbigg · 2022-10-26T07:59:02Z

packages/react-ui/src/atoms/Typography/index.js

@@ -0,0 +1,20 @@
+import React from 'react';


I don't know if that folder atoms/Typography is going to include major submodule with several files, but looks like no.
So, i would question if there is reason to implement it in file that name says nothing - index.js looks like "nothing", it's just not searchable, there are hundreds of them in workspace and IMO
I would kindly propose to name it just src/atoms/Typography.js and only split into several files if it grows big and unmaintainable.

TLDR: I find folder with lone index.js inconvenient and premature optimization.

Ok, I understand, changed!

vmilan · 2022-10-26T10:32:43Z

packages/react-ui/storybook/stories/widgetsUI/WrapperWidgetUI.stories.js

-import { Typography } from '@mui/material';
 import ColorizeIcon from '@mui/icons-material/Colorize';
 import MenuIcon from '@mui/icons-material/Menu';
 import AddLocationIcon from '@mui/icons-material/AddLocation';
 import WrapperWidgetUI from '.../../../src/widgets/WrapperWidgetUI';


Unused imports

zbigg · 2022-10-27T07:35:14Z

packages/react-ui/src/atoms/Typography.js

+    <MuiTypography
+      {...otherProps}
+      ref={ref}
+      style={{ fontWeight: FontWeight[weight], fontStyle: italic && 'italic' }}


Please include also style prop from user

In this shape, user is completly unable to overwrite style as we're ignoring /overwriting it.

Just putting the otherProps at the end would be enough, wouldn't it?

Just putting the otherProps

@VictorVelarde
It wouldn't work,

...otherProps at end would overwrite this component style

style from ...otherProps at beginning is overwritten in this version

Proper "merging" of properties "touched" by component must be manual, like this:

function Outer({foo, style, className, ...otherProps}) { return <Inner {...otherProps} className={clsx(foo && 'outerRoot', className)} style={{color: 'red', ...style}} /> }

Done, now styles overrides are working fine.

Yeah, I know about override behaviour @zbigg ... my point here is Does it make sense to restrict otherProps just to style prop and be explicit about it in the code?

I was assuming here that if someone wants to play with style, beyond defaults, they can provide externally all they require (so no mix, but simple overriding), and allowing us to play with other props. Eg, check Typograghy options in https://mui.com/material-ui/api/typography/ and think about gutterBottom, noWrap or others.

Having said that, I'm not super worried about this, we can move on and adapt as required later, if we see common use-cases

I was assuming here that if someone wants to play with style, beyond defaults, they can provide externally all they require

I would assume differently and would expect our "design system" components behave like proper MUI components. In this case, that "user provided style" and "props-based style" are properly merged if they reasonably not collide.

For example with normal Typography, if you say <Typography "color="primary" style={{cursor: "help"}}> it just works and we don't care how typography "implements" color.

Nor, we're required to implement "color" by ourselves if we use "style".

provide externally all they require

That would require user to know that weight: "normal" translates to style={{fontWeight: 500}} and that removes half of usefulness of this component.
(and workarounds for this would add even more api surface, like helper functions, exposing FontWeight mapping ...)

I think that robust APIs should ensure what unrelated features are indeed unrelated (as long as it's possible).

Josmorsot · 2022-10-27T07:35:42Z

packages/react-ui/storybook/stories/atoms/Palette.stories.js

+            variant='caption'
+            noWrap
+            className={classes.textValue}
+            ref={textRef}


why are you adding ref here?

Because of this: https://mui.com/material-ui/guides/composition/#caveat-with-refs

Tooltip (and a few other Mui components) needs to pass the ref to its children. I had to add the forwardRef in Typography to do that because is a custom component.

Without this, I was having this warning in the console:
Function components cannot be given refs. Attempts to access this ref will fail. Did you mean to use React.forwardRef()?

zbigg · 2022-10-27T07:35:59Z

packages/react-ui/storybook/stories/atoms/Typography.stories.js

+      description: 'Any theme color, default and custom ones',
+      control: {
+        type: 'select',
+        options: ['primary.dark', 'brand.navyBlue', 'default.main']


Where is definition of all those colors available ?

(again, Typography.d.ts would be great)

Are coming from the theme directly, you can use any color that we have declared there.

This is just a small example to have in the story, just to know you can change the color via props in the Typography component.

I guess that should be expressed in the typings

color is a common prop to Mui components (it's a system property)... do you want to restrict it to only theme colors?

Right now, from what I've seen, it accepts any string. I mean, whether a primary.main or a #000000, for example.

I think should be enough as it is, but let me know.

Ok, now i see that in MUI5 it's much more robust than in MUI4. In MUI 4, only very few hardcoded colors were accepted ('primary', ...)

Thanks for explanation.

Josmorsot

Nice work! LGTM 🚀

zbigg

LGTM, Apart from all comments (mostly explorative) i like it very much and we should make more components like this.

vmilan · 2022-10-27T15:47:47Z

LGTM, Apart from all comments (mostly explorative) i like it very much and we should make more components like this.

Thanks so much for the feedback, it really helps me.

shortcut-integration · 2022-10-28T08:16:27Z

This pull request has been linked to Shortcut Story #265549: CORE - Create a wrapper for the MUI Typography component.

vmilan added 2 commits October 25, 2022 09:28

Typography component to extend MUI Typography with new props

d81e439

basi extend of Typography and some replacements

0da6e2f

zbigg reviewed Oct 26, 2022

View reviewed changes

vmilan added 2 commits October 26, 2022 11:40

replace imports

cb2ec6c

component improves and clean up

b5e740b

vmilan commented Oct 26, 2022

View reviewed changes

vmilan added 5 commits October 26, 2022 12:39

clean up

ef630cb

typo

420e45f

merge

4d0a6c5

fix forwardRef

05b1dc7

fix oneOf propType

888af8f

vmilan mentioned this pull request Oct 27, 2022

New Design system implementation + MUI5 upgrade #494

Merged

types

396578e

vmilan marked this pull request as ready for review October 27, 2022 07:14

vmilan requested a review from a team October 27, 2022 07:15

zbigg reviewed Oct 27, 2022

View reviewed changes

Josmorsot reviewed Oct 27, 2022

View reviewed changes

zbigg reviewed Oct 27, 2022

View reviewed changes

Josmorsot approved these changes Oct 27, 2022

View reviewed changes

vmilan added 2 commits October 27, 2022 14:06

fixing style overrides

8c7bde4

fixing style overrides

5df5e91

zbigg approved these changes Oct 27, 2022

View reviewed changes

vmilan merged commit 3f26d2f into feature/design-system Oct 27, 2022

vmilan deleted the feature/typography-component branch October 27, 2022 15:50

vmilan added the design system label Dec 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Typography component #506

New Typography component #506

vmilan commented Oct 25, 2022 •

edited by VictorVelarde

Loading

zbigg left a comment

zbigg Oct 26, 2022

vmilan Oct 26, 2022

zbigg Oct 26, 2022

vmilan Oct 26, 2022

zbigg Oct 26, 2022

vmilan Oct 26, 2022

zbigg Oct 26, 2022

vmilan Oct 26, 2022

vmilan Oct 26, 2022

zbigg Oct 27, 2022

vmilan Oct 27, 2022

VictorVelarde Oct 27, 2022

zbigg Oct 27, 2022

vmilan Oct 27, 2022

VictorVelarde Oct 27, 2022 •

edited

Loading

zbigg Oct 27, 2022

Josmorsot Oct 27, 2022

vmilan Oct 27, 2022 •

edited

Loading

zbigg Oct 27, 2022

vmilan Oct 27, 2022

VictorVelarde Oct 27, 2022

vmilan Oct 27, 2022 •

edited

Loading

zbigg Oct 27, 2022

Josmorsot left a comment

zbigg left a comment

vmilan commented Oct 27, 2022

shortcut-integration bot commented Oct 28, 2022

New Typography component #506

New Typography component #506

Conversation

vmilan commented Oct 25, 2022 • edited by VictorVelarde Loading

Description

zbigg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VictorVelarde Oct 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vmilan Oct 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vmilan Oct 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Josmorsot left a comment

Choose a reason for hiding this comment

zbigg left a comment

Choose a reason for hiding this comment

vmilan commented Oct 27, 2022

shortcut-integration bot commented Oct 28, 2022

vmilan commented Oct 25, 2022 •

edited by VictorVelarde

Loading

VictorVelarde Oct 27, 2022 •

edited

Loading

vmilan Oct 27, 2022 •

edited

Loading

vmilan Oct 27, 2022 •

edited

Loading