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

Levelbuilder Clean Up: Code Area Editors #35037

Merged
merged 25 commits into from Jun 2, 2020
Merged

Levelbuilder Clean Up: Code Area Editors #35037

merged 25 commits into from Jun 2, 2020

Conversation

dmcavoy
Copy link
Contributor

@dmcavoy dmcavoy commented May 29, 2020

Similar to #35035 this groups together all the toolbox and code area editors together.

Droplet style level

code-area

Blockly Style level

code-area-blockly

@dmcavoy dmcavoy changed the base branch from staging to applab-edit-org May 29, 2020 17:04
@dmcavoy dmcavoy requested a review from a team as a code owner May 29, 2020 19:46
@davidsbailey
Copy link
Member

this doesn't look right -- the PR is full of commits from other parts of the staging branch. perhaps you just need to merge latest staging into this branch to fix it?

@dmcavoy
Copy link
Contributor Author

dmcavoy commented May 29, 2020

this doesn't look right -- the PR is full of commits from other parts of the staging branch. perhaps you just need to merge latest staging into this branch to fix it?

Sorry about that. I think I fixed it now

@dmcavoy dmcavoy removed the request for review from a team May 29, 2020 21:07
Comment on lines +8 to +12
- if @level.is_a? Applab
= render partial: 'levels/editors/fields/checkboxes', locals: {f: f, field_name: :makerlab_enabled, description: "Enable Maker APIs"}
= render partial: 'levels/editors/fields/checkboxes', locals: {f: f, field_name: :autocomplete_palette_apis_only, description: "Autocomplete palette APIs only"}
= render partial: 'levels/editors/fields/checkboxes', locals: {f: f, field_name: :execute_palette_apis_only, description: "Execute palette APIs only"}
= render partial: 'levels/editors/fields/checkboxes', locals: {f: f, field_name: :beginner_mode, description: "Beginner mode for loops"}
Copy link
Member

Choose a reason for hiding this comment

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

it's a little weird to see fields pulled out of an applab-specific partial, only to be put behind an applab conditional. but it's more important to arrange the fields the way that the curriculum team wants than it is to keep conditionals out of our code, so this still seems like an improvement to me.

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 agree. I'm kinda trying to organize in small chunks that make sense and I think there will be some moving things to possibly move them again in order to get this right. The mistake I made last time was trying to make it right on the first go and that was too hard to follow for reviews.

Copy link
Member

Choose a reason for hiding this comment

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

No worries, makes sense! FWIW the most vocal feedback I recall receiving last time was from Elizabeth objecting to having things being hidden by default, since that broke her flow of doing a text search to find the field that she was looking for. I think you're doing a fine job of avoiding that here by making all the expand/collapse controls be expanded by default.

@dmcavoy dmcavoy changed the base branch from applab-edit-org to staging May 30, 2020 16:33
@dmcavoy dmcavoy merged commit f9e21de into staging Jun 2, 2020
@dmcavoy dmcavoy deleted the code-area-edit branch June 2, 2020 21:37
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

3 participants