Skip to content

Migrate last slice of javalab redux to redux-toolkit - #51246

Merged
molly-moen merged 2 commits into
stagingfrom
molly/java-lab-final-redux-migration
Apr 12, 2023
Merged

Migrate last slice of javalab redux to redux-toolkit#51246
molly-moen merged 2 commits into
stagingfrom
molly/java-lab-final-redux-migration

Conversation

@molly-moen

Copy link
Copy Markdown
Contributor

Migration of the last slice of the java lab redux state to redux-toolkit. I also ran the typescript linter on the previous slices I created and fixed a couple issues there. There is no functionality change here.

Links

Testing story

Tested locally

PR Checklist:

  • Tests provide adequate coverage
  • Privacy and Security impacts have been assessed
  • Code is well-commented
  • New features are translatable or updates will not break translations
  • Relevant documentation has been added or updated
  • User impact is well-understood and desirable
  • Pull Request is labeled appropriately
  • Follow-up work items (including potential tech debt) are tracked and linked

@molly-moen
molly-moen requested a review from a team April 10, 2023 21:08
Comment thread apps/src/javalab/redux/javalabRedux.ts Outdated

@levadadenys levadadenys left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 minor comment, other than that LGTM!

isTesting: boolean;
backpackEnabled: boolean;
isStartMode: boolean;
levelName: string | undefined;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be simplified to levelName?: string

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO string | undefined might be clearer just so you have to explicitly assign levelName to undefined in the initial state, rather than omitting it?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah I think I'll leave it as-is so it's defined in initialState

@sanchitmalhotra126 sanchitmalhotra126 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@molly-moen
molly-moen merged commit e3b3768 into staging Apr 12, 2023
@molly-moen
molly-moen deleted the molly/java-lab-final-redux-migration branch April 12, 2023 16:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants