feat: CSS-in-JS pt 2 #712
feat: CSS-in-JS pt 2 #712
Conversation
muselesscreator
changed the title
| const _whiteMix = (color, perc) => _mix( | ||
| colors.white, colors[color], perc, | ||
| ); | ||
| const genColorLevels = (colorKey) => ({ |
There was a problem hiding this comment.
Does this produce the same hex values for all the colors as the original theme? (I'm not familiar with how it generates colors)
| _elementColorLevels = theme.elementColorLevels; | ||
| } | ||
|
|
||
| let baseColor = '#808080'; |
There was a problem hiding this comment.
Is this just a default in case something valid wasn't specified? Should it maybe be based on grey 500, rather than in between grey color levels?
| // Set the number of columns and specify the width of the gutters. | ||
| const grid = { | ||
| columns: 12, | ||
| gutterWidth: '30px', |
There was a problem hiding this comment.
Should this be expressed in terms of the spacer?
| marginY: spacer, | ||
| }, | ||
| mark: { | ||
| padding: '.2em', |
There was a problem hiding this comment.
Intentional that this is em instead of rem?
| import PropTypes from 'prop-types'; | ||
| import classNames from 'classnames'; | ||
| import { jsx, useTheme } from '@emotion/react'; | ||
| import { formControlSetStyle } from './style'; |
There was a problem hiding this comment.
Why put this in a separate file?
| const style = formControlSetStyle(useTheme()); | ||
| return jsx(as, { | ||
| className: classNames(className, 'pgn__form-control-set'), | ||
| css: isInline ? style.inline : style.base, |
There was a problem hiding this comment.
the css prop can be a function that accepts theme. It's probably better to use that form instead of grabbing the theme with useTheme
| css: isInline ? style.inline : style.base, | |
| css: (theme) => { | |
| const style = formControlSetStyle(theme); | |
| if (isInline) | |
| return style.inline; | |
| } | |
| return style.base; | |
| }, |
| const className = classNames(props.className); | ||
| const style = formTextStyle(useTheme()); | ||
| const css = { |
There was a problem hiding this comment.
Similar feedback/point of discussion as above.
| @@ -23,13 +29,13 @@ const ParagonProvider = ({ | |||
| ParagonProvider.propTypes = { | |||
| locale: PropTypes.string, | |||
| // eslint-disable-next-line react/forbid-prop-types | |||
| theme: PropTypes.object, | |||
| theme: PropTypes.func, | |||
There was a problem hiding this comment.
In this change you're suggesting theme must be a function?
| assertStatusAlertOpen(true); | ||
| wrapper.find('withDeprecatedProps(Button)').at(0).simulate( | ||
| 'keyDown', | ||
| { ...event, key: 'Enter' }, | ||
| ); | ||
| assertStatusAlertOpen(false); |
There was a problem hiding this comment.
Changes in this file seem unrelated to the purpose of this PR. Is there a reason this is all needed?
| bgLevel: -10, | ||
| borderLevel: -9, | ||
| colorLevel: 6, |
| // The default theme.js for Paragon. The structure of this object follows the | ||
| // theme specification found here: https://system-ui.com/theme/. We prefer to add | ||
| // to this theme over time rather than all valid keys at once to ensure everything | ||
| // present here is used in the system. |
There was a problem hiding this comment.
What exists below is a departure from the strategy articulated in this comment. And what's below doesn't follow the theme specification from system-ui. Let's discuss if this direct port of Bootstrap is desirable and if so, what next steps should be.
Migrates all scss variables into theme.js file, and modifies them with applyTheme method stored in brand repo (PRs active in both brand-openedx and brand-edx.org).
Uses emotion theme values to style the StatusAlert, FormText, and FormControlSet components.
JIRA:
https://openedx.atlassian.net/browse/PAR-450
https://openedx.atlassian.net/browse/PAR-451
https://openedx.atlassian.net/browse/PAR-452
https://openedx.atlassian.net/browse/PAR-454