fix: DatePicker styles with updated spec #1697

KevinGhadyani-Okta · 2023-02-22T22:13:21Z

Description

Updated DatePicker styles with updated spec.

edburyenegren-okta · 2023-02-27T20:10:49Z

packages/odyssey-react-labs/src/datePickerTheme.tsx

    transform: "translateY(-50%)",
+    width: `${(2 / 16) * (16 / 14)}rem`,


These won't hold if someone overrides typography.fontSize. What's the goal here? Maybe I can point you toward something safer.

For ex, we use em for our Status dots, so that they're always related to the size of the Status type:

"&::before": { content: "''", width: ".57em", height: ".57em", marginInlineEnd: theme.spacing(2), borderRadius: "100%", backgroundColor: theme.palette.text.secondary, }

(We should probably have variables for these dot sizes, but you get the idea.)

If someone overrides typography.fontSize, you're saying it won't scale up the width? Yeah, I'm assuming fonts sale with the HTML which is why I'm using rem.

But if someone modifies the MUI font size which doesn't modify the html element, then this 2px won't be consistent with the MUI font size right?

But I agree, they should be set as token variables, not 2px as rem.

Updated to this:

Suggested change

width: `${(2 / 16) * (16 / 14)}rem`,

width: '0.14285714285714285em', // 2px

Actually, changed it to this instead so someone else (or me) doesn't have to do the math by hand:

Suggested change

width: `${(2 / 16) * (16 / 14)}rem`,

width: `${(2 / 16) * (16 / 14)}em`,

This only works because the fontSize is FontScale1. I think I need to change 16 to use FontScale1, and we should be good, but then I'd be using calc() instead which causes other issues. If I had access to the pixel values of our fonts, it'd be a lot more reliable, but this does scale properly if the font-size changes.

edburyenegren-okta · 2023-02-27T20:14:47Z

packages/odyssey-react-labs/src/datePickerTheme.tsx

+  default: ({ theme }) => ({
+    backgroundColor: theme.palette.primary.main,
+    content: '""',
+    height: `${(16 / 16) * (16 / 14)}rem`,


Similar comment as above here. Could this be 1em or theme.typography.body.fontSize?

I made it relative to the fontSize. That makes sense here.

Suggested change

height: `${(16 / 16) * (16 / 14)}rem`,

height: theme.typography.body1.fontSize,

Actually, I changed this to h6 because 1rem is actually 14px rather than 16px to match the text. FontScale2. The icon needs to be sized 1 font size above the text to match the design.

Totally cool to use fontSize variables here. Another option I use sometimes is theme.typography.ui.lineHeight, which is unitless. You can tag an em on there to make it scale with the local font-size (which gives you 16px with 14px type).

packages/odyssey-react-labs/src/datePickerTheme.tsx

edburyenegren-okta · 2023-02-27T20:28:53Z

I missed this during the initial PR. At 141 you have width: ${(296 / 16) * (16 / 14)}rem, width: auto solves the same problem and should be safer.

KevinGhadyani-Okta · 2023-02-28T22:27:13Z

I missed this during the initial PR. At 141 you have width: ${(296 / 16) * (16 / 14)}rem, width: auto solves the same problem and should be safer.

I agree completely, but MUI requires a fixed height here because it's using position: absolute on the dates. I'd have to reconfigure the way dates render to remove this fixed height, and I'm not sure that's gonna be risky or not considering how much MUI's changing in the next version.

edburyenegren-okta · 2023-02-28T22:36:04Z

I'd have to reconfigure the way dates render to remove this fixed height[...]

This is for the width, not the height. Changing to width: auto gives the expected result:

KevinGhadyani-Okta · 2023-03-01T21:27:32Z

There's a few px difference in width from a fixed-width to 'auto'. I'll keep it at 'auto', but it might be wider than we expect. Probably, it's the correct size.

Look at how much farther out the navigation arrows appear:

'auto' also shrinks the Year view. Do we want it fixed-width or 'auto'?

edburyenegren-okta · 2023-03-02T01:26:28Z

Good question re: the shifting sizes between displays. I prefer my containers sized to content, but @josesolano-okta and @taylorridgway-okta may want a uniform experience when clicking through the picker.

taylorridgway-okta · 2023-03-02T21:09:24Z

Good question re: the shifting sizes between displays. I prefer my containers sized to content, but @josesolano-okta and @taylorridgway-okta may want a uniform experience when clicking through the picker.

As predicted, I'd prefer uniform width for both views if possible

edburyenegren-okta · 2023-03-03T21:08:20Z

packages/odyssey-react-labs/src/datePickerTheme.tsx

    content: '" "',
-    height: `${(2 / 16) * (16 / 14)}rem`,
+    height: `${(2 / 16) * (16 / 14)}em`,


Is this 2px in Figma? If so, can prolly leave it as such here. The (16 / 14) modifier won't make sense if someone rethemes the base fontSize, since the proportional adjustment will be different.

If we do want it to scale with the font-size, the calculated value might be better since it won't need to reference the global values at all. It's something Design should probably create a token for anyway.

width: `${(2 / {theme.typography.fontSize})}em`

* fix(odyssey-react-mui): don't require children prop for Icon buttons * fix: DatePicker styles with updated spec (#1697) * fix: added missing MuiThemeDecorator to DatePicker story * fix: adjusted spacing on DatePicker to match design * chore: updated yarn * fix: potential fix for local MUI and Labs development * fix: styled DatePicker to match new design spec * fix: add build before yarn start and fix order of yarn dev * fix: added missing DatePicker border * fix(odyssey-react-labs): improved selected vs "this year" styling * fix(odyssey-storybook): hover styles outside of month * fix(odyssey-storybook): removed MUI Month Picker styles * fix(odyssey-react-labs): added theme variables where they were hardcoded * fix(odyssey-react-labs): improved DatePicker styles to be more dynamic --------- Co-authored-by: Kamron Batmanghelich <kamron.batmanghelich@okta.com> * build: bump versions for v0.21.0 * docs: update CHANGELOG for v0.21.0 --------- Co-authored-by: Kevin Ghadyani <97464104+KevinGhadyani-Okta@users.noreply.github.com> Co-authored-by: Kamron Batmanghelich <kamron.batmanghelich@okta.com>

KevinGhadyani-Okta and others added 7 commits February 21, 2023 15:27

fix: added missing MuiThemeDecorator to DatePicker story

cf61aea

fix: adjusted spacing on DatePicker to match design

c52e379

chore: updated yarn

0765098

fix: potential fix for local MUI and Labs development

f713e45

fix: styled DatePicker to match new design spec

545a8f6

fix: add build before yarn start and fix order of yarn dev

8223fa0

fix: added missing DatePicker border

18ada9a

KevinGhadyani-Okta requested review from edburyenegren-okta, josesolano-okta and taylorridgway-okta February 22, 2023 22:13

KevinGhadyani-Okta requested review from a team, ganeshsomasundaram-okta and kamronbatmanghelich-okta as code owners February 22, 2023 22:13

fix(odyssey-react-labs): improved selected vs "this year" styling

4b81bb7

KevinGhadyani-Okta force-pushed the kg/OKTA-577942 branch from 2d699a3 to 4b81bb7 Compare February 27, 2023 17:36

edburyenegren-okta reviewed Feb 27, 2023

View reviewed changes

packages/odyssey-react-labs/src/datePickerTheme.tsx Outdated Show resolved Hide resolved

KevinGhadyani-Okta added 2 commits February 27, 2023 16:09

fix(odyssey-storybook): hover styles outside of month

381e6a5

fix(odyssey-storybook): removed MUI Month Picker styles

5a09bf7

fix(odyssey-react-labs): added theme variables where they were hardcoded

7a8050b

KevinGhadyani-Okta force-pushed the kg/OKTA-577942 branch 2 times, most recently from b9be9df to 3e004da Compare March 3, 2023 20:50

edburyenegren-okta reviewed Mar 3, 2023

View reviewed changes

fix(odyssey-react-labs): improved DatePicker styles to be more dynamic

bf7e783

KevinGhadyani-Okta force-pushed the kg/OKTA-577942 branch from 3e004da to bf7e783 Compare March 3, 2023 22:20

edburyenegren-okta approved these changes Mar 3, 2023

View reviewed changes

KevinGhadyani-Okta merged commit cba0894 into develop Mar 8, 2023

KevinGhadyani-Okta deleted the kg/OKTA-577942 branch March 8, 2023 16:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: DatePicker styles with updated spec #1697

fix: DatePicker styles with updated spec #1697

KevinGhadyani-Okta commented Feb 22, 2023

edburyenegren-okta Feb 27, 2023

edburyenegren-okta Feb 27, 2023

KevinGhadyani-Okta Feb 28, 2023 •

edited

Loading

KevinGhadyani-Okta Mar 1, 2023 •

edited

Loading

edburyenegren-okta Feb 27, 2023

KevinGhadyani-Okta Feb 28, 2023 •

edited

Loading

KevinGhadyani-Okta Mar 1, 2023 •

edited

Loading

edburyenegren-okta Mar 2, 2023

edburyenegren-okta commented Feb 27, 2023

KevinGhadyani-Okta commented Feb 28, 2023 •

edited

Loading

edburyenegren-okta commented Feb 28, 2023 •

edited

Loading

KevinGhadyani-Okta commented Mar 1, 2023 •

edited

Loading

edburyenegren-okta commented Mar 2, 2023

taylorridgway-okta commented Mar 2, 2023

edburyenegren-okta Mar 3, 2023

edburyenegren-okta Mar 3, 2023 •

edited

Loading

		transform: "translateY(-50%)",
		width: `${(2 / 16) * (16 / 14)}rem`,

	width: `${(2 / 16) * (16 / 14)}rem`,
	width: '0.14285714285714285em', // 2px

	width: `${(2 / 16) * (16 / 14)}rem`,
	width: `${(2 / 16) * (16 / 14)}em`,

	height: `${(16 / 16) * (16 / 14)}rem`,
	height: theme.typography.body1.fontSize,

fix: DatePicker styles with updated spec #1697

fix: DatePicker styles with updated spec #1697

Conversation

KevinGhadyani-Okta commented Feb 22, 2023

Description

edburyenegren-okta Feb 27, 2023

Choose a reason for hiding this comment

edburyenegren-okta Feb 27, 2023

Choose a reason for hiding this comment

KevinGhadyani-Okta Feb 28, 2023 • edited Loading

Choose a reason for hiding this comment

KevinGhadyani-Okta Mar 1, 2023 • edited Loading

Choose a reason for hiding this comment

edburyenegren-okta Feb 27, 2023

Choose a reason for hiding this comment

KevinGhadyani-Okta Feb 28, 2023 • edited Loading

Choose a reason for hiding this comment

KevinGhadyani-Okta Mar 1, 2023 • edited Loading

Choose a reason for hiding this comment

edburyenegren-okta Mar 2, 2023

Choose a reason for hiding this comment

edburyenegren-okta commented Feb 27, 2023

KevinGhadyani-Okta commented Feb 28, 2023 • edited Loading

edburyenegren-okta commented Feb 28, 2023 • edited Loading

KevinGhadyani-Okta commented Mar 1, 2023 • edited Loading

edburyenegren-okta commented Mar 2, 2023

taylorridgway-okta commented Mar 2, 2023

edburyenegren-okta Mar 3, 2023

Choose a reason for hiding this comment

edburyenegren-okta Mar 3, 2023 • edited Loading

Choose a reason for hiding this comment

KevinGhadyani-Okta Feb 28, 2023 •

edited

Loading

KevinGhadyani-Okta Mar 1, 2023 •

edited

Loading

KevinGhadyani-Okta Feb 28, 2023 •

edited

Loading

KevinGhadyani-Okta Mar 1, 2023 •

edited

Loading

KevinGhadyani-Okta commented Feb 28, 2023 •

edited

Loading

edburyenegren-okta commented Feb 28, 2023 •

edited

Loading

KevinGhadyani-Okta commented Mar 1, 2023 •

edited

Loading

edburyenegren-okta Mar 3, 2023 •

edited

Loading