-
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
Translations and polish for unlimited points #1665
base: main
Are you sure you want to change the base?
Conversation
GeraldRequired Reviewers
Don't want to be involved in this pull request? Comment |
Size Change: +72 B (+0.01%) Total Size: 863 kB
ℹ️ View Unchanged
|
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.
There's a change to the reducer that looks like it might have been accidental. Otherwise, this looks good!
I also left a non-blocking suggestion inline about replacing the strings
prop with usePerseusI18n
.
@@ -74,7 +74,8 @@ export function interactiveGraphReducer( | |||
): InteractiveGraphState { | |||
switch (action.type) { | |||
case REINITIALIZE: | |||
return initializeGraphState(action.params); | |||
return state; | |||
// return initializeGraphState(action.params); |
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.
Pretty sure this change shouldn't be here, right?
@@ -358,13 +368,19 @@ const renderGraphControls = (props: { | |||
state: InteractiveGraphState; | |||
dispatch: (action: InteractiveGraphAction) => unknown; | |||
width: number; | |||
strings: PerseusStrings; |
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.
Instead of adding a prop for the strings, could we call usePerseusI18n
here too? Or is that not possible / advisable? It seems like using the context might be a bit more convenient.
Summary:
Issue: https://khanacademy.atlassian.net/browse/LEMS-1670
Test plan: