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

Katleiah/gb danceparty fielddropdown #44313

Merged
merged 4 commits into from Jan 13, 2022

Conversation

katleiahramos
Copy link
Contributor

Existing error:
image

Links

Jira Ticket

Testing story

Manually Testing:

  1. Pull down branch locally and start app
  2. Visit any dance party level between 6-10. example: http://localhost-studio.code.org:3000/s/dance-2019/lessons/1/levels/6?blocklyVersion=Google
  3. Ensure that level works as expected and there are no console errors.

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

Comment on lines 27 to 35
const isMissingSecondValue = menuGenerator.some(
entry => entry.length === 1
);

if (isMissingSecondValue) {
menuGenerator = menuGenerator.map(entry => {
return [entry[0], entry[0]];
});
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think Ideally we could move a lot of this logic out of the constructor and into their own methods, but when I did that I got an error that I couldn't call this before super(). Open to ideas, but also ok leaving it here if that ends up making the most sense.

@katleiahramos katleiahramos requested a review from a team January 11, 2022 19:01
@katleiahramos katleiahramos force-pushed the katleiah/gb-danceparty-fielddropdown branch from 0705e89 to 4354c6d Compare January 12, 2022 18:31
Comment on lines 1340 to 1346
dropdownOptions.forEach(option => {
if (option.length === 1) {
newOptions.push([option[0], option[0]]);
} else {
newOptions.push(option);
}
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is a slightly more performant way to handle the logic. Let me know if you think this change makes sense!

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: might be slightly cleaner as a map like return options.map(option => option.length === 1 ? [option[0], option[0]] : option)

Copy link
Contributor

Choose a reason for hiding this comment

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

I like doing this in a separate function so that we can have a doc comment for it :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

great idea! updated here: eed89ab

/**
* Adds a second value to options array elements if a second one does not exist.
* The second value is used as the generated code for that option.
* Also required for backwards compatibility with existing blocks that are missing the second value.
Copy link
Contributor

Choose a reason for hiding this comment

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

tiny nit: remove "also"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated here: eed89ab!

@katleiahramos katleiahramos merged commit 127dde7 into staging Jan 13, 2022
@katleiahramos katleiahramos deleted the katleiah/gb-danceparty-fielddropdown branch January 13, 2022 18:19
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