-
Notifications
You must be signed in to change notification settings - Fork 19
[terra-dev-site-v7] Use TERRA_THEME_CONFIG
for dev site theming config
#303
Conversation
'@babel/plugin-proposal-object-rest-spread', | ||
'@babel/plugin-transform-object-assign', | ||
'@babel/plugin-transform-regenerator', | ||
'@babel/plugin-transform-runtime', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added to allow the use of async/await in jest
@@ -5,21 +5,13 @@ const html = fs.readFileSync(require.resolve('./head.html'), 'utf8'); | |||
|
|||
const siteConfig = { | |||
navConfig, | |||
sideEffectImports: ['./terra-dev-site-test-theme.scss'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed this test theme
@@ -148,14 +148,16 @@ | |||
"peerDependencies": { | |||
"react-dom": "^16.8.5", | |||
"react": "^16.8.5", | |||
"terra-toolkit": "^5.21.0 || ^6.0.0", | |||
"terra-toolkit": "^6.1.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bumped to ensure TERRA_THEME_CONFIG
available
@@ -9,11 +9,8 @@ const generateUtilitiesConfig = (appConfig) => { | |||
// build out the menu items for themes, locals, and bidi. | |||
const config = { | |||
defaultTheme: appConfig.defaultTheme, | |||
themes: appConfig.themes || {}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now defaulted in _AppSettingsProvider
const scopedThemes = themesConfig?.scoped || []; | ||
const defaultThemeName = themesConfig?.theme || 'terra-default-theme'; | ||
|
||
const themesMap = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Map theme name to css class... generally one to one except for the default theme
} = settingsConfig; | ||
|
||
if (Array.isArray(settingsConfig.locales) && settingsConfig.locales.length !== 0) { | ||
// eslint-disable-next-line no-console | ||
console.warn('Locale configurations are deprecated as of terra-dev-site v6.23.0. You may remove this setting from your configuration files.'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed warning, from last mvb
@@ -26,8 +26,6 @@ Terra.describeViewports('utilities', ['huge'], () => { | |||
browser.click('#terra-dev-site-locale-select'); | |||
browser.click('#terra-select-option-en-AU'); | |||
|
|||
browser.click('#terra-dev-site-theme-select'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed, now that we're relying on the TERRA_THEME_CONFIG
global, it changes the available themes when tests are run with an override theme. So this select menu is not reliably available for all tests.
// defaultTheme: 'terra-default-theme', | ||
|
||
/* The default locale of the site. 'en' is the default theme. */ | ||
defaultLocale: 'en', | ||
// defaultLocale: 'en', | ||
|
||
/* The default direction of the site. 'ltr' is the default direction. */ | ||
defaultDirection: 'ltr', | ||
// defaultDirection: 'ltr', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's still valid for people to set a default theme, locale or direction through the site.config.js, but they don't have to.... i left that there because this file doubles as our documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add a little more info around where this default config comes from? The fact its setup in webpack is probably a little hidden
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I'll add some more documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added here: 7f3377d
src/navigation/_SettingsPicker.jsx
Outdated
const appSettings = React.useContext(AppSettingsContext); | ||
const [state, setState] = useState({ locale: appSettings.currentLocale, theme: appSettings.currentTheme, direction: appSettings.currentDirection }); | ||
const { | ||
locale, theme, direction, | ||
} = state; | ||
const themes = Object.keys(config.themes); | ||
const { themes } = appSettings; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we extract directions here as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, and locales too
*/ | ||
const locales = typeof TERRA_AGGREGATED_LOCALES === 'object' ? TERRA_AGGREGATED_LOCALES : ['en']; | ||
const themesConfig = typeof TERRA_THEME_CONFIG === 'object' ? TERRA_THEME_CONFIG : {}; | ||
const scopedThemes = themesConfig?.scoped || []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need optional chaining if we are defaulting to {}?
if typeof TERRA_THEME_CONFIG === 'object'
could be either an object or array, does the opitonal chaining handle this gracefully?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, i suppose not.
…config into site config
Summary
TERRA_THEME_CONFIG
.themes
has been removed from site config.TERRA_THEME_CONFIG
is only available in terra-toolkit v6.1.0.locales
has been removed from site config. As of 6.23 Locales are pulled from the globalTERRA_AGGREGATED_LOCALES
variable provided by terra-toolkit v5.21.0.Related to: #294
Deployment Link
https://terra-dev-site-deployed-pr-#.herokuapp.com/
Testing
Additional Details
Thank you for contributing to Terra.
@cerner/terra