Music: proposal for LevelData variants#52570
Conversation
|
|
||
| @merged_properties = @level.properties.camelize_keys.merge({video: @video&.camelize_keys}) | ||
| @properties = @level.properties.camelize_keys | ||
| @properties[:levelData] = @video if @video |
There was a problem hiding this comment.
I was also thinking about this for a separate change; as this gets more complex, should we just add a summarize_for_lab2 function on level.rb where we can craft the structure of the payload we want to send, and move the logic out of the controller?
There was a problem hiding this comment.
Oh, that is a good idea, and definitely the right path forward.
There was a problem hiding this comment.
Added a summarize_for_labs2_properties, though flexible on naming.
…deo-levels-with-video-data-as-level-data
| export interface LevelVideo { | ||
| // The level data for a standalone_video level that doesn't require | ||
| // reloads between levels. | ||
| export interface LevelDataVideo { |
There was a problem hiding this comment.
Could we instead make this a VideoLevelData that extends LevelData, similar to MusicLevelData? That way the video specific properties can live closer to the video specific components?
There was a problem hiding this comment.
Should anything belong in LevelData in that case? It feels like text, validations, and startSources are rather specific to be in there...
There was a problem hiding this comment.
Hmm, yeah I felt like text/validations/startSources were somewhat generic, but i guess they are specific to levels with progress and projects. Maybe we should rename LevelData to ProjectLevelData? Then yeah we can keep it like you have it where type LevelData = ProjectLevelData | VideoLevelData. Not sure what's best but I'm sure we'll refine this as we support different level types
| } | ||
| end | ||
|
|
||
| def summarize_for_lab2_properties |
There was a problem hiding this comment.
nit: mind adding a doc for reference here?
There was a problem hiding this comment.
Oh, and just realized, we'd need to update levels_controller with this as well right?
There was a problem hiding this comment.
Good point. Just fixed in parent branch.
As it happens, the individual level pages still worked fine because we didn't change what level_properties returns for a music level, and we're using the legacy server-rendered page for a standalone video.
No description provided.