Skip to content
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

[Poetry][HOC] Save selected poem in progress #42871

Merged
merged 2 commits into from
Oct 11, 2021
Merged

[Poetry][HOC] Save selected poem in progress #42871

merged 2 commits into from
Oct 11, 2021

Conversation

ajpal
Copy link
Contributor

@ajpal ajpal commented Oct 6, 2021

Heavily modeled after #25466

Saves the selected poem in the project sources so that it persists on page reload and share

Initial level load with default poem:
image

Share view with selected poem:
image

After remix with custom poem:
image

@ajpal ajpal requested a review from a team October 6, 2021 23:54
Comment on lines 12 to 16
if (config.level.selectedPoem) {
getStore().dispatch(setPoem(config.level.selectedPoem));
} else if (this.level.defaultPoem) {
getStore().dispatch(setPoem(POEMS[this.level.defaultPoem]));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

this is subjective, but i think we can simplify this bit of code:

const poem = config.level.selectedPoem || POEMS[this.level.defaultPoem];
if (poem) {
  getStore().dispatch(poem);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, yeah, I like that better! thanks!

@@ -130,6 +130,10 @@ function initProjects() {
getSelectedSong: function() {
return false;
},
setSelectedPoem: function() {},
Copy link
Contributor

Choose a reason for hiding this comment

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

i've run into this situation before (having to stub sourceHandler on pixelation) -- we should not have to do this 🙃 i've logged STAR-1785 to refactor this away

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for logging that!

Base automatically changed from oct4-share-remix to staging October 11, 2021 16:59
@ajpal ajpal merged commit 9dd29bd into staging Oct 11, 2021
@ajpal ajpal deleted the oct6-save-poem branch October 11, 2021 17:02
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.

None yet

2 participants