-
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 truncate directive, Modularize slide toc buttons #489
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/issue-416 |
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: 0 of 6 files reviewed, 1 unresolved discussion (waiting on @IshavSohal)
a discussion (no related file):
Very nice work! Honestly, this should've been done in the original PR, sorry for offloading it.
An issue I noticed is that highlighting seems to be broken. The EN/FR config buttons don't get a highlight when selected, and the slide option buttons (copy, delete, move up/down) don't highlight upon hover.
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.
Code looks a lot cleaner, nice work!
Reviewed 5 of 6 files at r1, all commit messages.
Reviewable status: 5 of 6 files reviewed, 4 unresolved discussions (waiting on @gordlin and @IshavSohal)
src/components/helpers/slide-toc-button.vue
line 42 at r1 (raw file):
<!-- Options for ::lang:: items with missing configs (e.g. one language has config, other doesn't) --> <div v-if="!element[lang]" class="ml-auto flex my-auto"> <!-- Create a new blank config -->
The Create new blank config
button doesn't seem to be working when clicked!
src/components/helpers/slide-toc-button.vue
line 128 at r1 (raw file):
// assigning the content using a ternary expression prevents the 'editor.slide.toc.noFRSlide' // from being detected if (this.element[this.lang]?.title) {
I'm thinking that since this code block is repeated twice, it may be beneficial to just create a function for it and then call it in mounted()
and updated()
.
src/components/helpers/slide-toc-button.vue
line 132 at r1 (raw file):
} else if (this.element[this.lang]?.title === '') { if (this.lang === 'en') { this.content = this.$t('editor.slides.toc.newENGSlideText');
Totally up to you, but this can also be written as the following to remove the extra if
block:
this.content = this.lang === 'en' ? this.$t('editor.slides.toc.newENGSlideText') : this.$t('editor.slides.toc.newFRSlideText');
Same applies to the else
block below.
c9af343
to
cc5db75
Compare
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: 4 of 6 files reviewed, 4 unresolved discussions (waiting on @gordlin and @RyanCoulsonCA)
a discussion (no related file):
Previously, gordlin (Gordon Lin) wrote…
Very nice work! Honestly, this should've been done in the original PR, sorry for offloading it.
An issue I noticed is that highlighting seems to be broken. The EN/FR config buttons don't get a highlight when selected, and the slide option buttons (copy, delete, move up/down) don't highlight upon hover.
Donethanks
src/components/helpers/slide-toc-button.vue
line 42 at r1 (raw file):
Previously, RyanCoulsonCA (Ryan Coulson) wrote…
The
Create new blank config
button doesn't seem to be working when clicked!
Donethanks
src/components/helpers/slide-toc-button.vue
line 128 at r1 (raw file):
Previously, RyanCoulsonCA (Ryan Coulson) wrote…
I'm thinking that since this code block is repeated twice, it may be beneficial to just create a function for it and then call it in
mounted()
andupdated()
.
Donethanks
src/components/helpers/slide-toc-button.vue
line 132 at r1 (raw file):
Previously, RyanCoulsonCA (Ryan Coulson) wrote…
Totally up to you, but this can also be written as the following to remove the extra
if
block:this.content = this.lang === 'en' ? this.$t('editor.slides.toc.newENGSlideText') : this.$t('editor.slides.toc.newFRSlideText');
Same applies to the
else
block below.
Donethanks
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: 4 of 6 files reviewed, 4 unresolved discussions (waiting on @IshavSohal and @RyanCoulsonCA)
a discussion (no related file):
Previously, IshavSohal (Ishav Sohal) wrote…
Donethanks
Button highlighting is fixed, but the EN/FR config highlighting still seems to be broken:
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: 4 of 6 files reviewed, 4 unresolved discussions (waiting on @gordlin and @RyanCoulsonCA)
a discussion (no related file):
Previously, gordlin (Gordon Lin) wrote…
Button highlighting is fixed, but the EN/FR config highlighting still seems to be broken:
Donethanks
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 4 of 6 files at r1, 1 of 2 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @RyanCoulsonCA)
Related Item(s)
#416
#488
Changes
Testing
Steps:
...
) button for a slide ToC button, and ensure that theClear content
andCopy from other config
features still work correctlyCopy Slide
feature still works correctlyThis change is