Levelbuilder Clean Up: Playlab edit #35089
Conversation
… in studio and calc levels for cs in a
| - if @level.respond_to? :coordinate_grid_background | ||
| .field | ||
| = render partial: 'levels/editors/fields/checkboxes', locals: {f: f, field_name: :coordinate_grid_background, description: "Coordinate grid background"} | ||
| = render partial: 'levels/editors/fields/validation_code', locals: {f: f} if (@level.uses_droplet?) || @level.is_a?(Blockly) |
There was a problem hiding this comment.
I think it's fine to do here since we're already doing it so much, but the proliferation of these type checks is worrying to me in general. It'd be worth thinking about if there's a better approach that lets us control things on a per-level basis without needing to scatter conditions throughout the code base.
There was a problem hiding this comment.
Good call @uponthesun ... I agree , especially when there are multiple dimensions to the checks, such as class types as well as whether the level is a droplet level. here are some ideas:
-
A simple (but at this point time-consuming) change to the current approach would be to nest all of the partials according to the class structure. so, levels/editors/_applab.html.haml would include levels/editors/_blockly.html.haml, which would include app/views/levels/editors/_all.html.haml (possibly renamed to _level.html.haml). this gives us simplicity, but puts some restrictions on the way we lay out the level edit page. for example, if we have debugger controls which are applab specific and others which are more general, then it would be hard to show those on the same part of the page.
-
Another idea would be to have one giant template for editing levels, containing a place to display each level edit option. then each level type would say whether it cared about that field, or, ideally, we could check if the level type had the property or column corresponding to the field and use that to decide whether to display it.
-
it's possible some hybrid approach is possible where we take the simpler approach (1) overall, but in cases where we want to group certain types of controls more tightly, we use approach 2 on just that section/partial.
anyway... since we're 20 PRs down the current path right now I don't think we want to redo this at the moment, but let's plan to follow a more deliberate pattern here when we move to any new UI for level editing. ideally the solution will move to react, leaving most of this code behind.
There was a problem hiding this comment.
Thanks for the feedback. My actual plan is to get ride of the all partial soon and move each of these fields into the specific level files once things are more cleaned up and clear what should live where. I know its a little bit of a weird path to get there but its the one I could wrap my head around how to slowly move over instead of huge PRs that might be hard to follow. Anyway hopefully in future PRs you will see an improvement here.
There was a problem hiding this comment.
Sounds great Dani. If you go that approach, you could still extract some shared partials as needed. but in general I'm very much in favor of each level editor page having its own partial which pulls in any shared stuff it needs, rather than every level page pulling in the same shared partial which then decides what to pull in based on the level type.
1. Coordinate Grid moved to the CS in A area as it is only used on Studio and Calc level types which are types that include the CS in A partial
2. Move playlab validation editors to the validation area
3. Group together Playlab sprites and collision editors