Skip to content

Split Blockly#blockly_options into blockly_app_options and blockly_level_options#9300

Merged
balderdash merged 8 commits into
stagingfrom
multi-level-options
Jul 5, 2016
Merged

Split Blockly#blockly_options into blockly_app_options and blockly_level_options#9300
balderdash merged 8 commits into
stagingfrom
multi-level-options

Conversation

@balderdash

Copy link
Copy Markdown
Contributor

Working toward generating blockly_level_options multiple times.

Comment thread dashboard/app/models/blockly.rb Outdated

def blockly_app_options(game, skin_id)
options = Rails.cache.fetch("#{cache_key}/blockly_app_options/v2") do

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Style/EmptyLinesAroundBlockBody: Extra empty line detected at block body beginning.

@Hamms

Hamms commented Jul 5, 2016

Copy link
Copy Markdown
Contributor

I love seeing a refactor like this that ultimately adds so few lines!

Didja talk to Will about the current app/level options split?

Comment thread dashboard/app/models/blockly.rb Outdated
app: game.try(:app),
droplet: game.try(:uses_droplet?),
pretty: Rails.configuration.pretty_apps ? '' : '.min',
})

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.

I believe our styleguide prefers these to be all left-aligned:

app_options.merge!({
  baseUrl: Blockly.base_url,
  app: game.try(:app),
  droplet: game.try(:uses_droplet?),
  pretty: Rails.configuration.pretty_apps ? '' : '.min',
})

@Hamms

Hamms commented Jul 5, 2016

Copy link
Copy Markdown
Contributor

This look quite good, but I'd love to chat briefly about the motivation behind this change just to make sure I actually understand the context.

@balderdash

Copy link
Copy Markdown
Contributor Author

Talked to Will about the view_options/level_view_options split a bit. It seems the original intent for the split has degraded a bit as people added config options, which sounds about right to me. I'm just going to continue cleaning that up.

As a sanity check, I compared appOptions before and after this change on a random level (/course4/stage/16/puzzle/4). Aside from timestamps and a couple props being reordered, they're identical.

@Hamms

Hamms commented Jul 5, 2016

Copy link
Copy Markdown
Contributor

LGTM!

@balderdash balderdash merged commit 72caabc into staging Jul 5, 2016
@balderdash balderdash deleted the multi-level-options branch July 6, 2016 20:57
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