-
Notifications
You must be signed in to change notification settings - Fork 481
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
[Spritelab] Make instructions icon configurable #41772
Conversation
apps/src/p5lab/P5Lab.js
Outdated
const avatar = config.level.instructionsIcon || 'avatar'; | ||
this.skin.smallStaticAvatar = `${MEDIA_URL}${avatar}.png`; | ||
this.skin.staticAvatar = `${MEDIA_URL}${avatar}.png`; | ||
this.skin.winAvatar = `${MEDIA_URL}${avatar}.png`; | ||
this.skin.failureAvatar = `${MEDIA_URL}${avatar}.png`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: i think this would be more readable if we set ${MEDIA_URL}${avatar}.png
to a variable and reused that. something like
const avatarUrl = `${MEDIA_URL}${config.level.instructionsIcon || 'avatar'}.png`
this.skin.smallStaticAvatar = avatarUrl;
// set other avatar skins
@@ -13,7 +13,26 @@ | |||
%div{ style: 'background-color: rgb(239, 239, 239); padding-top: 12px' } | |||
~ f.text_area :long_instructions, rows: 4 | |||
|
|||
- if @level.is_a? GamelabJr | |||
= f.label :instructions_icon,'Instructions Icon' | |||
%p Please refer to #{link_to 'Sprite Lab Static Assets', 'https://github.com/code-dot-org/code-dot-org/tree/staging/apps/static/spritelab'} for available assets, then enter the name of the image to use as the instructions icon for this level. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure how likely this is, but we could run into a situation where the assets listed in apps/static/spritelab/
on the staging branch aren't available on levelbuilder yet. we could point this URL to the production (or levelbuilder) branch instead of staging?
var iconName = $('#level_instructions_icon')[0].value; | ||
if (iconName) { | ||
$('.instructions-icon-preview').attr('src', 'https://studio.code.org/blockly/media/spritelab/' + iconName + '.png'); | ||
} else { | ||
$('.instructions-icon-preview').attr('src', 'https://studio.code.org/blockly/media/spritelab/avatar.png'); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: i think this would be more readable if we set a fallback for iconName, which is essentially what we're doing below, but would allow us to set the src attribute once
var iconName = $('#level_instructions_icon')[0].value || 'avatar';
$('.instructions-icon-preview').attr('src', 'https://studio.code.org/blockly/media/spritelab/' + iconName + '.png');
Make the instructions icon configurable per level. This will only work for assets in https://github.com/code-dot-org/code-dot-org/tree/staging/apps/static/spritelab but it's easy enough to add new assets there as needed. Having all the assets served from there seems preferable over using any URL for two reasons:
Edit page:
Before (always turtle)
After (configurable per level)
Links
jira
Testing story
Deployment strategy
Follow-up work
Privacy
Security
Caching
PR Checklist: