feat!: add design tokens support#759
Conversation
BREAKING CHANGE: Pre-design-tokens theming is no longer supported.
4046498 to
3cfcd2c
Compare
The shell's SCSS must be explicitly loaded by site.config.dev.tsx.
346657a to
72e7b20
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## frontend-base #759 +/- ##
================================================
Coverage ? 88.79%
================================================
Files ? 158
Lines ? 1294
Branches ? 214
================================================
Hits ? 1149
Misses ? 141
Partials ? 4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@brian-smith-tcril, thanks to @jesusbalderramawgu's latest commit, this now passes tests. Ready for review! |
brian-smith-tcril
left a comment
There was a problem hiding this comment.
Overall this is looking great!
I haven't tested this locally yet, but I have
- Compared the changes here to frontend-base...master
- Looked at arbrandes#1 for more context
I found some places where the changes didn't match up and it wasn't clear why, and a couple small issues, but nothing major.
| jest.mock('./ViewCourseButton', () => 'ViewCourseButton'); | ||
| jest.mock('./BeginCourseButton', () => 'BeginCourseButton'); | ||
| jest.mock('./ResumeButton', () => 'ResumeButton'); | ||
| jest.mock('../../../../slots/CourseCardActionSlot', () => jest.fn(() => <div>CourseCardActionSlot</div>)); |
There was a problem hiding this comment.
| jest.mock('../../../../slots/CourseCardActionSlot', () => jest.fn(() => <div>CourseCardActionSlot</div>)); | |
| jest.mock('@src/slots/CourseCardActionSlot', () => jest.fn(() => <div>CourseCardActionSlot</div>)); |
src/setupTest.jsx
Outdated
| jest.mock('./data/constants/app', () => ({ | ||
| ...jest.requireActual('./data/constants/app'), | ||
| locationId: 'fake-location-id', | ||
| })); |
There was a problem hiding this comment.
Is this needed? It looks like it was removed on master in #678
There was a problem hiding this comment.
Yeah, it's not needed. I'll remove it.
src/setupTest.jsx
Outdated
| jest.mock('./utils', () => ({ | ||
| ...jest.requireActual('./utils'), | ||
| nullMethod: jest.fn().mockName('utils.nullMethod'), | ||
| })); |
There was a problem hiding this comment.
Is this needed? It looks like it was removed on master in #678
src/setupTest.jsx
Outdated
There was a problem hiding this comment.
Is this needed? It looks like it was removed on master in #678
| it('logs error with target, name, and error stack', () => { | ||
| // eslint-ignore-next-line no-unused-vars | ||
| const callBadKey = () => dict.fakeKey; | ||
| callBadKey(); | ||
| expect(window.console.error.mock.calls).toEqual([ | ||
| [{ target: dict, name: 'fakeKey' }], | ||
| [Error('invalid property "fakeKey"').stack], | ||
| ]); | ||
| it('returns undefined for missing key', () => { | ||
| // Accessing a missing key should return undefined | ||
| expect(dict.fakeKey).toBeUndefined(); |
There was a problem hiding this comment.
It looks like this used to test error logging but now it's just testing for undefined in a slightly different way than it is being tested for a couple lines down?
There was a problem hiding this comment.
Removal was intentional, but the test is redundant.
| describe('useInitializeDashboard', () => { | ||
| it('dispatches initialize thunk action on component load', () => { | ||
| hooks.useInitializeDashboard(); | ||
| const [cb, prereqs] = React.useEffect.mock.calls[0]; | ||
| expect(prereqs).toEqual([]); | ||
| expect(initializeApp).not.toHaveBeenCalled(); | ||
| cb(); | ||
| expect(initializeApp).toHaveBeenCalledWith(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
It looks like @jesusbalderramawgu's comment about removing this (arbrandes#1 (comment)) didn't get a response, so I'm assuming it's intentional.
There was a problem hiding this comment.
This is something I left for after we get fully into react-query land, since Jesus had some trouble writing tests that covered the initialization function fully.
| describe('useCardDetailsData', () => { | ||
| const providerData = { | ||
| name: 'my-provider-name', | ||
| name: 'Unknown', |
There was a problem hiding this comment.
This feels like it might be hiding an issue with the "forwards provider name if it exists, else formatted unknown provider name" by matching the unknownProviderName
There was a problem hiding this comment.
Yeah, we shouldn't be passing 'Uknown' here. I'll look into it.
| import track from '../../../../tracking'; | ||
| import { StrictDict } from '../../../../utils'; |
There was a problem hiding this comment.
Can these use @src?
| reduxHooks: { useMasqueradeData: jest.fn(), useCardEnrollmentData: jest.fn() }, | ||
| jest.mock('@src/hooks', () => ({ | ||
| reduxHooks: { | ||
|
|
There was a problem hiding this comment.
nit: empty line here
| if (isMasquerading) { | ||
| it('is disabled', () => { | ||
| expect(loadToggle().props.disabled).toEqual(true); | ||
| it('renders when masquerading', () => { | ||
| const emailSettingsButton = screen.getByRole('button', { name: messages.emailSettings.defaultMessage }); | ||
| expect(emailSettingsButton).toBeInTheDocument(); | ||
| expect(emailSettingsButton).toHaveAttribute('aria-disabled', 'true'); | ||
| expect(emailSettingsButton).toHaveClass('disabled'); | ||
| }); | ||
| } else { | ||
| it('is enabled', () => { | ||
| expect(loadToggle().props.disabled).toEqual(false); | ||
| const emailSettingsButton = screen.getByRole('button', { name: messages.emailSettings.defaultMessage }); | ||
| expect(emailSettingsButton).toBeEnabled(); | ||
| }); |
There was a problem hiding this comment.
the naming here feels a bit confusing.
There was a problem hiding this comment.
Yeah, we should change the name back to "is disabled".
|
Tested locally with diff --git a/site.config.dev.tsx b/site.config.dev.tsx
index cf10014..ac25c9a 100644
--- a/site.config.dev.tsx
+++ b/site.config.dev.tsx
@@ -36,6 +36,17 @@ const siteConfig: SiteConfig = {
],
accessTokenCookieName: 'edx-jwt-cookie-header-payload',
+
+ theme: {
+ defaults: {
+ light: "light",
+ },
+ variants: {
+ light: {
+ url: "https://cdn.jsdelivr.net/gh/brian-smith-tcril/sample-plugin@brand/brand/dist/light.min.css",
+ },
+ },
+ },
};
export default siteConfig;and everything seems to work as expected! Once the comments are addressed this should be good to merge! |
BREAKING CHANGE: Pre-design-tokens theming is no longer supported.
Depends on openedx/frontend-base#111.