-
Notifications
You must be signed in to change notification settings - Fork 357
Removes the width override for the cs-program widget #2753
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
base: main
Are you sure you want to change the base?
Conversation
🗄️ Schema Change: No Changes ✅ |
Size Change: -14 B (0%) Total Size: 496 kB
ℹ️ View Unchanged
|
🛠️ Item Splitting: No Changes ✅ |
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (a72e615) and published it to npm. You Example: pnpm add @khanacademy/perseus@PR2753 If you are working in Khan Academy's frontend, you can run the below command. ./dev/tools/bump_perseus_version.ts -t PR2753 If you are working in Khan Academy's webapp, you can run the below command. ./dev/tools/bump_perseus_version.js -t PR2753 |
Just for context: are you saying there was no visual impact for KA Classic but it helped fix a bug in Classroom? That's nice! My main concern then, if that's the case, is the impact on mobile...but I'm not sure how much we support CSProgram on mobile. 🤔 Might be worth taking a look though. |
Luke found a bit more context for the original override which was added almost 7 years ago. It was because some buttons were rendering off screen. I added a screen recording and some extra suggestions here in Slack. |
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 change introduces a regression for classic CS programs embedded in articles, but that is an acceptable tradeoff to improve the new Classroom experience.
@@ -174,11 +174,6 @@ class CSProgram extends React.Component<Props> implements Widget { | |||
} | |||
|
|||
const styles = StyleSheet.create({ | |||
// Override the inherited width from the perseus paragraph class |
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.
Since the removal of lines doesn't leave blame traces, can you include a code comment noting that removing a width override to 820px exposed a layout issue with rendering CS programs in articles, but this was preferable to leaving the override which introduced a layout issue in the new Classroom view.
Summary:
Remove width override for cs-program widget. This width was causing some overflow issues in KA Classroom, and after testing this in Classroom and Classic, I don't think it's needed.
Classic
Screen.Recording.2025-08-01.at.3.11.24.PM.mov
Classroom
Screen.Recording.2025-08-01.at.3.08.36.PM.mov
Issue: TUT-2719
Test plan: