-
Notifications
You must be signed in to change notification settings - Fork 13
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
add intro background customization to metadata editor #505
base: main
Are you sure you want to change the base?
Conversation
Your demo site is ready! 🚀 Visit it here: https://ramp4-pcar4.github.io/storylines-editor/fix-494 |
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.
Nice addition! Seems like it'll be a pretty useful feature.
Reviewed 2 of 5 files at r1, all commit messages.
Reviewable status: 2 of 5 files reviewed, 4 unresolved discussions (waiting on @RyanCoulsonCA)
src/components/helpers/metadata-content.vue
line 222 at r1 (raw file):
:alt="$t('editor.introBackgroundPreview')" /> <p v-if="metadata.logoPreview == 'error'" class="image-preview">
I think you forgot to change the error; it's still connected to logoPreview
's state. If the background image has a preview, its error will follow the error that the logo preview has.
When the logo doesn't have an error:
src/components/helpers/metadata-content.vue
line 235 at r1 (raw file):
style="display: none" /> </section>
Does the background image need an alt text field like the logo?
src/lang/lang.csv
line 73 at r1 (raw file):
editor.metadataForm.caption.introSubtitle,"The intro subtitle displays underneath the intro title, in small text.",1,"Le sous-titre d'introduction s'affiche sous le titre d'introduction, en petit texte.",0 editor.metadataForm.caption.logoAltText,"For accessibility purposes, provide description text for the logo.",1,"À des fins d’accessibilité, veuillez fournir un texte de description pour le logo.",0 editor.metadataForm.caption.introBackground,"The background image is displayed on the first slide of the storyline product, behind the intro title and subtitle.",1,"L'image d'arrière-plan est affichée sur la première diapositive du produit de scénario, derrière le titre et le sous-titre d'introduction.",0
This caption hasn't been added to the background image form item:
src/lang/lang.csv
line 110 at r1 (raw file):
editor.introBackground,Background image,1,Image d'arrière-plan,0 editor.introBackgroundPreview,Background image preview,1,Aperçu de l'image d'arrière-plan,0 editor.introBackgroundPreview,Preview of background image,1,Aperçu de l'image d'arrière-plan,0
Seems that editor.introBackgroundPreview
is duplicated.
Of the two, I recommend keeping Background image preview
, as it matches the logo text's structure (Logo preview
).
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.
Reviewable status: 2 of 5 files reviewed, 4 unresolved discussions (waiting on @gordlin)
src/components/helpers/metadata-content.vue
line 222 at r1 (raw file):
Previously, gordlin (Gordon Lin) wrote…
I think you forgot to change the error; it's still connected to
logoPreview
's state. If the background image has a preview, its error will follow the error that the logo preview has.
Great catch, fixed!
src/components/helpers/metadata-content.vue
line 235 at r1 (raw file):
Previously, gordlin (Gordon Lin) wrote…
Does the background image need an alt text field like the logo?
It shouldn't need it as we have role="presentation"
on all of the background images in the app, which basically tells screen readers that they can happily ignore it as it's just for aesthetics.
src/lang/lang.csv
line 73 at r1 (raw file):
Previously, gordlin (Gordon Lin) wrote…
This caption hasn't been added to the background image form item:
Nice catch, added it in.
src/lang/lang.csv
line 110 at r1 (raw file):
Previously, gordlin (Gordon Lin) wrote…
Seems that
editor.introBackgroundPreview
is duplicated.Of the two, I recommend keeping
Background image preview
, as it matches the logo text's structure (Logo preview
).
Fixed! I blame Github Copilot for this, it's having too much fun auto-filling in code that I don't want 🤖.
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.
Reviewed 1 of 5 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @RyanCoulsonCA)
src/components/helpers/metadata-content.vue
line 235 at r1 (raw file):
Previously, RyanCoulsonCA (Ryan Coulson) wrote…
It shouldn't need it as we have
role="presentation"
on all of the background images in the app, which basically tells screen readers that they can happily ignore it as it's just for aesthetics.
Makes sense!
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.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @RyanCoulsonCA)
a discussion (no related file):
Good stuff! Noticed a fairly minor issue with both the logo and background upload. Try uploading an image, then removing it, then uploading the same image again. Notice that the image doesn't get uploaded. Once you upload a different image and remove that, you should be able to upload the original image again.
Since this bug isn't specific to this PR, we can just open a new issue if it isn't a straightforward fix.
Related Item(s)
#494
Changes
metadata-editor.vue
to be less logo-specific so I could reuse them for the intro background as well.Notes
This PR doesn't add support for modifying the text colour of the title, subtitle, and "scroll to story" button, which Storylines does support.
I will create a new issue and begin working on this next.PR can be found here.Testing
Steps:
This change is