[Theme] - Border Radius

[daigof]

• Review APP: http://curology-radiance-pr-642.herokuapp.com (some components to look for: Modals, Tooltips, Dropdown, Field, Buttons, Alerts, option button, Indicator, Chip)

• Refactor all full circles radius to 50% for consistency: https://www.codecademy.com/forum_questions/559fe347e39efe4cf40005a9

• Change Container message radius from 16px to theme medium (8px)

• Named exports for some constants (e.g. SPACER) for better intellisense and consistency

• added Border Radius story

• Changed Toggle focus border radius to theme large: was hardcoded 12px (beyond 12px the rounded visual is the same so no impact in primary theme). This affects secondary theme, which focus as square
  

it is more aligned to the theme but let me know if this needs to be changed back and hardcoded as rounded focus

[michaeljaltamirano]

Since this PR is concerned with implementing the style guide prescription for how border-radius should operate, I think we should define an enum for what the acceptable border-radius values are, in order to actually enforce it.

It could get thorny because most user-land usage is probably in rem but we're using mostly pixels in this PR, but something like this might work:

type BorderRadiusValues = valueof<ThemeType['BORDER_RADIUS']> | '0.25rem' | '0.5rem' | '2rem'

[benkolde]

@daigof I see two ways forward with the toggle

Either square off the toggle, or round the focus. I think I'd like to round the focus for now, and then revisit the design later

[michaeljaltamirano]

@daigof I see two ways forward with the toggle

Either square off the toggle, or round the focus. I think I'd like to round the focus for now, and then revisit the design later

@daigof The toggle is a dependency (react-toggle-button) that is probably our biggest tech debt in this repo. Let's keep the focus by hard-coding for now. We can add the appropriate // TODO or JSDoc comments to memorialize the hard-coding as appropriate.

[michaeljaltamirano]

Approving without requesting changes but would like to see the following before merging this in:

1. Keep the border-radius for Toggle and add TODOs as needed
2. Move more border-radius theming logic into the styled component logic instead of the main component body.
3. Updating the BORDER_RADIUS story to show divs (containers, etc.) that show the border-radius in effect.
4. Enforcing a strict set of valid border-radius values so that we can guide users in the user-land to style guide-approved happy paths. (e.g. type BorderRadiusValues = valueof<ThemeType['BORDER_RADIUS']> | '0.25rem' | '0.5rem' | '2rem').

I've accepted the Container visual regressions (for messageType) as the new baseline so future commits should pass that part of the build step.

After those changes, we should be good to go with releasing a minor version. Nothing is breaking though the stricter type might requires some refactors in user-land.

[daigof]

@michaeljaltamirano I did 1-3 items, but I disagree with number 4). Historically you can just pass whatever value here it was never mentioned to be strict to 4/8/32. So you could theoretically pass down a 16px or whatever you like. I don't understand why start enforcing strict values now

[michaeljaltamirano]

@michaeljaltamirano I did 1-3 items, but I disagree with number 4). Historically you can just pass whatever value here it was never mentioned to be strict to 4/8/32. So you could theoretically pass down a 16px or whatever you like. I don't understand why start enforcing strict values now

We do the same thing for COLORS, etc. The component library should inform (and, courtesy of TypeScript, enforce) how our common components are used. If something is outside of the use case, then it should be explicitly documented in both the library and design style guides.

I did not request changes because I don't think it is mandatory, but I think it is something we need to be much stricter about--especially as the team grows--so that we do not end up with inconsistent code patterns in user-land.

[daigof]

ok I enforced the new type