-
Notifications
You must be signed in to change notification settings - Fork 351
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
Remove z-index tokens, constants, and classes that place SOME widgets above the doodlepad. #2170
Conversation
I'll do some testing once I get an npm artifact. |
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (25a97be) and published it to npm. You Example: yarn add @khanacademy/perseus@PR2170 If you are working in Khan Academy's webapp, you can run: ./dev/tools/bump_perseus_version.sh -t PR2170 |
Size Change: -162 B (-0.01%) Total Size: 1.48 MB
ℹ️ View Unchanged
|
@@ -416,7 +416,7 @@ class Graph extends React.Component<Props> { | |||
|
|||
return ( | |||
<div | |||
className="graphie-container above-scratchpad" | |||
className="graphie-container blank-background" |
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.
Swapped because above-scratchpad
adds blank-background
styles in perseus-renderer.less
@@ -161,7 +161,7 @@ class Measurer extends React.Component<Props> implements Widget { | |||
<div | |||
className={ | |||
"perseus-widget perseus-widget-measurer " + | |||
"graphie-container above-scratchpad" | |||
"graphie-container blank-background" |
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.
Here too, ensuring we don't lose nested styles
whoops my branch is completely mis-named. oh well |
Works as expected! Screen.Recording.2025-01-30.at.11.55.58.AM.mov |
@MikeKlemarewski added you mostly as FYI |
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'm not currently able to test this in webapp myself due to some dev env issues with bumping Perseus, but this looks sound to me!
I only had one nit about the possibility of fully deleting the deprecated style.
It would be good to test the Numeric Input / Input Number widgets to make sure everything is working as expected (as they did use the Interactive style), but I think we're good to go otherwise. :) I don't think we have many, if any, uses of the Plotter widget anymore.
Thank you for a great improvement! Not sure why we ever tried to show widgets above the scratchpad myself.
packages/perseus/src/perseus-api.tsx
Outdated
@@ -168,6 +168,9 @@ export const ClassNames = { | |||
SELECTED: "perseus-radio-selected", | |||
OPTION_CONTENT: "perseus-radio-option-content", | |||
}, | |||
/** | |||
* @deprecated This used to add a z-index of 3. Now it does nothing. | |||
*/ | |||
INTERACTIVE: "perseus-interactive", |
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'm a little less familiar with this file, but would it be possible to remove this style now? If the style is doing nothing now, it seems like it could be a fairly safe thing to clean up!
The only consumers of it are the widgets already in this PR, along with two more:
- Input with Examples (A sub component of Numeric Input / Input Number that is used for desktop views)
- Plotter
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 is in the API so I was hesitant to make any breaking changes.
I wasn't sure about the purpose of this enum. Could this be used as a selector? If so, maybe I should restore the class and just delete the styles.
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'll explore uses of this enum, but it would be a breaking change, which would lead to a major version bump.
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 don't see any consumers of ClassNames so I think it's safe to remove it.
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 guess the Perseus API is internal? At least this was. Removed it.
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.
Excellent call, Sarah
…long-standing bugs in consuming code.
7f8abaf
to
9f494ca
Compare
Summary
I did this in two passes -- one commit to remove
above scratchpad
, and a second forinteractive
, which seems to be used for the same purpose. Both place widgets above thescratchpaddoodlepad, which has long led to many bugs.Issue: TUT-1059