Add music lab to featured projects#58973
Conversation
thomasoniii
left a comment
There was a problem hiding this comment.
suggestion about adding an abstraction to getting the thumbnail url, but otherwise lgtm 🚀
|
|
||
| export const getThumbnailUrl = ( | ||
| path: string, | ||
| type: keyof typeof PROJECT_DEFAULT_THUMBNAIL_IMAGE_OVERRIDE |
There was a problem hiding this comment.
Trying to understand TS...isn't type often going to be something other than music (which I think is the only valid value given the contents of the constant)? I guess it compiles but TS confuses me sometimes...
There was a problem hiding this comment.
Good question - I tried string at first and TS didn't like it. I'll ping Sanchit when he's back in office next week to get his thoughts. Thanks!
There was a problem hiding this comment.
Tagging @sanchitmalhotra126 now that he's back in office.
There was a problem hiding this comment.
Hmm yeah, I think because PROJECT_DEFAULT_THUMBNAIL_IMAGE_OVERRIDE is defined in a JS file, TS is reading its type to be strictly {[key: 'music']: string} rather than a generic {[key: string]: string} and therefore complains when you try to use a string to index into it.
One solution would be to define this constant in TS where you can explicitly type it as a {[key: string]: string} and then you can have the type param be a string. Otherwise it might just be best to cast the param inside the function - have the type param be string and cast it to keyof typeof PROJECT_DEFAULT_THUMBNAIL_IMAGE_OVERRIDE internally.
Using keyof typeof for the type param does technically compile but only because all the references to this function are in JS. If/when a TS file tries to use this function, it will fail to compile unless the provided type is always 'music'.
There was a problem hiding this comment.
Thanks Sanchit! I converted projectConstants to a .ts file. Lmk what you think.
| <ProjectAppTypeArea | ||
| labKey="featured" | ||
| labName={i18n.featuredProjects()} | ||
| labViewMoreString={i18n.projectTypeDanceViewMore()} |
There was a problem hiding this comment.
What does this change do?
There was a problem hiding this comment.
The public gallery update included not showing the 'View more of ...' button. So this value is not being used since hideViewMoreLink has the value of true. I left a comment here.
| navigateFunction={this.onSelectApp} | ||
| isDetailView={false} | ||
| hideViewMoreLink={true} | ||
| hideWithoutThumbnails={true} |
There was a problem hiding this comment.
Does this change have any potential impact on other lab types?
There was a problem hiding this comment.
Since the public gallery only displays curated featured projects, admin will select projects for which thumbnail images are available. At '/projects/featured' (the admin featured projects page), the thumbnail images are displayed alongside a link to the project.
I believe in the past, since any project could be published, this was a way to filter projects without thumbnail images from being included in the public gallery (projects without any content).
As new labs are added, either they'll have a way to generate a corresponding thumbnail OR we'll need to provide a thumbnail image as we are doing for music lab. To my knowledge, legacy labs that are publishable generate thumbnail images.
| // Project data from the channels API. | ||
| type project = { |
There was a problem hiding this comment.
Hmm, we have a version of this interface in lab2 types (where it's called Channel) and I'm wondering if we want to reuse/consolidate...ideally it would be great to have a single source of truth for these types (so there's a consistent understanding of what a "project" is across the codebase) but probably out of scope of this PR to do that kind of refactor. Maybe ok to keep here for now but add a comment that this type is duplicated in lab2/types? cc @molly-moen
also nit: capitalize Projects
There was a problem hiding this comment.
Oh good to know about Channel interface - thanks Sanchit! I updated the interface adding two optional properties needed for convertChanneslToProjectData. @molly-moen lmk if this looks okay to you. If not, I can revert this commit. Thanks!
There was a problem hiding this comment.
The other option that @sanchitmalhotra126 and I discussed was converting the new projectUtils.ts file to a .js file. 🤷♀️
There was a problem hiding this comment.
Seems fine to add those fields there, they already exist on the channel itself, right?
There was a problem hiding this comment.
Yes, they do. Thanks!
This PR allows music lab projects to be added to the featured project gallery. Because thumbnail images are not generated for music lab projects, a customized thumbnail image is used for the featured projects public gallery and the admin featured projects page. However, the music lab image for the public gallery (via the project card) is slightly different in that more space is allowed for the music note.
An aside - at first, I also removed
studiotype labs from featured project types because it is no longer possible to convert astudiofree-play level to a standalone project. But it is still possible to create astudiostandalone project 'from scratch' so reverted the removal.Note that a project still needs to be in a 'published' state in order for it to be displayed in the public featured gallery. Follow-up work includes providing a way for an admin user to publish a project since currently an engineer is needed to publish a project.
This is part 1 for the task: Music: Ability to select featured projects to show on the home page (code.org/music). Part 2 is to retrieve the channel ids of the active music lab featured projects to use for the mini-player at code.org/music.
After update
Screenshot of public featured project gallery:
Screenshot of music lab projects after being added as featured projects at /projects/featured.
Links
Slack thread
Testing story
I tested locally by creating two different music lab project, published them, then made them active featured projects using dashboard-console commands.
Deployment strategy
Follow-up work
Privacy
Security
Caching
PR Checklist: