-
Notifications
You must be signed in to change notification settings - Fork 350
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Storybook] Configure Aphrodite to Not Append !important to Styles #2107
base: main
Are you sure you want to change the base?
[Storybook] Configure Aphrodite to Not Append !important to Styles #2107
Conversation
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (ba8bb5a) and published it to npm. You Example: yarn add @khanacademy/perseus@PR2107 If you are working in Khan Academy's webapp, you can run: ./dev/tools/bump_perseus_version.sh -t PR2107 |
Size Change: +3 B (0%) Total Size: 1.48 MB
ℹ️ View Unchanged
|
.storybook/main.ts
Outdated
// Once the LESS files have cascade layers included (a more involved task for a later time), | ||
// then the following plugin option should be removed. |
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.
Thanks for documenting the exit path. Do you think it's worth creating a ticket and noting it here?
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.
I vote for a ticket created and linked here so we don't forget. 😊
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.
Done.
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.
Approved, added a note about linking a ticket for follow-up.
.storybook/main.ts
Outdated
// Once the LESS files have cascade layers included (a more involved task for a later time), | ||
// then the following plugin option should be removed. |
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.
I vote for a ticket created and linked here so we don't forget. 😊
Adjust styling logic to use classes instead of inline styling.
// particular, at large zoom levels this line height does almost | ||
// nothing, but at the default size this shifts the letter down one | ||
// pixel so it is much better centered. | ||
lineHeight: "1px", |
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.
This doesn't appear to have any effect other than when isolated in a Storybook example. Removing it in an actual exercise has no effect.
Summary:
Aphrodite appends
!important
to all styling, by default. This is cumbersome for debugging and for writing appropriately defined rulesets. This configuration change causes Aphrodite to NOT append!important
. This change is limited to our dev environments (i.e. Storybook). Webapp has already implemented this, and handles Perseus styling accordingly.Additionally, this change uncovered a bug that causes an incorrect option to display as blue instead of red. The styling logic was refactored to account for the removal of
!important
and to clarify how styling is applied based upon the widget options.Issue: LEMS-2227
Test plan:
!important
suffixes.Test plan for bugfix: