Implement ToC redesign/refactor, both-language previews

[gordlin]

Related Item(s)

Issues:
#401
#402

Changes

• [FEATURE] Implements complete redesign and refactor of ToC, based on designs given by UX team.
  • Both language configs are displayed simultaneously, removing the need to swap configs. Creating a new slide creates two new configs, in English and French.
  • New slide item design, reducing clutter. Each title can take up to two lines now, meaning most titles are fully visible immediately, without tooltips.
  • ToC will now smooth-scroll automatically to the new slide when you create one. It will also smooth scroll to a copied slide until it's in the middle of the ToC, to give a little indicator that something happened.
  • UI element redesign (e.g. buttons)
  • Indicators for slides with a missing config (e.g. a slide doesn't have a FR config). Code handles all edge cases (that I could think of) involving a slide missing one of its language configs. Slides without a config can copy the config of its slide neighbour (e.g. FR version of Slide 12, if it has no config, can copy ENG config of slide 12 if it exists) OR create a new blank config.
  • Slides allow for deleting an individual lang config, as long as its the last of its lang configs in the list (e.g. you can delete an ENG config if its the last ENG config, i.e. there are no ENG configs in any slide below it).
  • ToC should no longer overflow the viewport height, requiring two scrollbars to be used (main page + ToC-specific) to scroll it.
  • A separate ToC is created for mobile sizes. When the viewport is small enough, the ToC will be auto-hidden. Clicking the new hamburger menu up top will open a new mobile ToC drawer, featuring larger buttons and more visual dividers to make it better for touch devices. To access an item's tooltips, you simply long-press it to simulate a hover. Dragging slides to reorder is disabled so users can scroll properly.
• [FEATURE] Implements new preview button, allowing user to select which language config they want to preview.
• [REFACTOR] Complete refactor of code involving slides/configs, to handle new ToC.

Notes

Implementing the new ToC required a change in how the slides object worked. It's now called bothLanguageSlides, and each contains two Slide objects, one for each config, as so:

export interface SlideForBothLanguages {
    en: Slide | undefined;
    fr: Slide | undefined;
}

App layout: I've modified the layout so the whole app no longer overshoots the page height (and requires a scrollbar); scrolling is now done only for individual panes (sidebar or slide editor+feedback), when they independently overflow. However, my current solution hardcodes the page height based on the existing elements; it works for different viewport sizes, but will break if there's ANY added/removed page elements in the future that increases/decreases the size given for the ToC or the slide editor half of the page. A future app layout refactor should probably find a better way to do this.

TODO

• Mobile ToC (this one doesn't shrink)
• Comment main code

Testing

Steps:

1. Open the product 00000000-0000-0000-0000-000000000000.
2. View the new ToC. Test each of the new ToC features listed above, in both the English and French versions of the page.
3. Use the new Preview dropdown to preview both the English and French configs. They should both still show even unsaved changes.

---

This change is 

[github-actions]

Your demo site is ready! 🚀 Visit it here: https://ramp4-pcar4.github.io/storylines-editor/redesign-toc

[gordlin]

Reviewable status: 0 of 8 files reviewed, 1 unresolved discussion

a discussion (no related file):
Added a mobile version of the ToC.

Now, the ToC will be auto-hidden on mobile. Clicking the new hamburger menu up top will open a new mobile ToC drawer, featuring larger buttons and more visual dividers to make it better for touch devices. To access an item's tooltips, you simply long-press it to simulate a hover. Dragging slides to reorder is disabled so users can scroll properly.

---

[gordlin]

Reviewable status: 0 of 8 files reviewed, 2 unresolved discussions

a discussion (no related file):
Added reasonably comprehensive comments to editor.vue and slide-toc.vue.

---

[gordlin]

Reviewable status: 0 of 8 files reviewed, 3 unresolved discussions

a discussion (no related file):
Ready for review!

---

[yileifeng]

Overall huge enhancements with the new UI, also great work to get it presentable on mobile as well which was not a priority previously 👍

A few bugs I found:

• selecting the advanced editor on a FR slide and then navigating back to any panel will turn the corresponding English slide into FR:
  
• I believe tooltips for the panel titles in ToC should only be visible for truncated text
• what is the use case of the delete last config for each lang? May be confusing to users for only one slide to have this option whereas other slides do not
• related to above, need a confirmation before deleting a panel

Additional suggestion - perhaps it would be beneficial to move the template code for mobile ToC to its own Vue component if it is feasible to avoid over cluttering one component, will leave that up to you

Reviewed 2 of 8 files at r1.
Reviewable status: 2 of 8 files reviewed, 14 unresolved discussions (waiting on @gordlin)

---

src/components/editor.vue line 373 at r3 (raw file):

    @Prop() metadata!: MetadataContent;

    @Prop() bothLanguageSlides!: SlideForBothLanguages[];

Would suggest just to keep the naming of this to be slides for clarity

---

src/components/editor.vue line 374 at r3 (raw file):

    @Prop() bothLanguageSlides!: SlideForBothLanguages[];
    @Prop() slides!: Slide[];

This does not look like it is used anymore

---

src/components/editor.vue line 455 at r3 (raw file):

        setTimeout(() => {
            if (

To clean up this code a bit for readability, some suggestions:

• first check if (index == -1 || !this.loadSlides) -> this.currentSlide = ''
• use some helper function or declare: const selectedLang = lang ?? (this.configLang as keyof SlideForBothLanguages) followed by const selectedSlide = this.loadSlides[index][selectedLang]
• then we can just set in the else block: this.currentSlide = selectedSlide ?? '';

Might need small modifications but this seems like it should work to me

---

src/components/metadata-editor.vue line 251 at r3 (raw file):

                :sourceCounts="sourceCounts"
                :metadata="metadata"
                :bothLanguageSlides="bothLanguageSlides"

same as above, recommend change to slides

---

src/components/metadata-editor.vue line 459 at r3 (raw file):

                const logoSrc = `assets/${this.configLang}/${this.metadata.logoName}`;

                const frSlides = props.configs.en?.slides.map((engSlide) => {

How come frSlides are being assigned English slides here and vice versa?

---

src/components/metadata-editor.vue line 470 at r3 (raw file):

                });

                const maxLength = Math.max(frSlides!.length ?? 0, engSlides!.length ?? 0);

Change to const maxLength = frSlides.length > enSlides.length ? frSlides.length : enSlides.length. If there are TS complaints, can default the eng and fr slides to [].

---

src/components/metadata-editor.vue line 937 at r3 (raw file):

        });

        const maxLength = Math.max(frSlides!.length ?? 0, engSlides!.length ?? 0);

Same comment as above, but since this is the same block of logic just throw this in a function to avoid duplicate code

---

src/components/slide-toc.vue line 271 at r3 (raw file):

                                            <button
                                                class="slide-toc-button"
                                                v-if="!bothLanguageSlides.slice(index + 1).some((slide) => slide.en)"

change to bothLanguageSlides.slice(index + 1).every((slide) => slide.fr)

---

src/components/slide-toc.vue line 462 at r3 (raw file):

                                            <button
                                                class="slide-toc-button"
                                                v-if="!bothLanguageSlides.slice(index + 1).some((slide) => slide.fr)"

change to bothLanguageSlides.slice(index + 1).every((slide) => slide.en)

---

src/components/slide-toc.vue line 617 at r3 (raw file):

})
export default class SlideTocV extends Vue {
    @Prop() bothLanguageSlides!: SlideForBothLanguages[];

same suggestion change back to slides

---

src/components/slide-toc.vue line 648 at r3 (raw file):

                    : {
                          title: '',
                          panel: [

Set this default slide as a local variable to avoid repeating

[gordlin]

selecting the advanced editor on a FR slide and then navigating back to any panel will turn the corresponding English slide into FR

Donethanks!

I believe tooltips for the panel titles in ToC should only be visible for truncated text

Tooltips appearing for all panel titles was the default behaviour before too (notice the v-truncate directive from RAMP isn't in this repo). I can implement this if you think it's worthwhile.

what is the use case of the delete last config for each lang? May be confusing to users for only one slide to have this option whereas other slides do not

There are a few issues that pop up now that we're always showing both configs. Because they're saved independently (as they probably should), the ENG/FR config objects are each meant to be a continuous list of existing slides, without random holes. This means there can't really be "isolated" configs. For example: if Slide 12's French config doesn't exist, but Slide 14 has a French config, then upon save Slide 14's French slide is automatically "slid up" to slide 12 and any holes will be deleted, which could be really confusing if the user expects their slot with no config to stay there. As such, only the final config for each language can be safely deleted without weird behaviour like this occurring.

related to above, need a confirmation before deleting a panel

Donethanks!

Dismissed @yileifeng from 2 discussions.
Reviewable status: 2 of 8 files reviewed, 12 unresolved discussions (waiting on @yileifeng)

---

src/components/editor.vue line 373 at r3 (raw file):

Previously, yileifeng (Yi Lei Feng) wrote…

Would suggest just to keep the naming of this to be slides for clarity

Donethanks!

---

src/components/editor.vue line 374 at r3 (raw file):

Previously, yileifeng (Yi Lei Feng) wrote…

This does not look like it is used anymore

Deleted, donethanks!

---

src/components/editor.vue line 455 at r3 (raw file):

Previously, yileifeng (Yi Lei Feng) wrote…

To clean up this code a bit for readability, some suggestions:

• first check if (index == -1 || !this.loadSlides) -> this.currentSlide = ''
• use some helper function or declare: const selectedLang = lang ?? (this.configLang as keyof SlideForBothLanguages) followed by const selectedSlide = this.loadSlides[index][selectedLang]
• then we can just set in the else block: this.currentSlide = selectedSlide ?? '';

Might need small modifications but this seems like it should work to me

Donethanks!

---

src/components/metadata-editor.vue line 251 at r3 (raw file):

Previously, yileifeng (Yi Lei Feng) wrote…

same as above, recommend change to slides

Donethanks!

---

src/components/metadata-editor.vue line 459 at r3 (raw file):

Previously, yileifeng (Yi Lei Feng) wrote…

How come frSlides are being assigned English slides here and vice versa?

Whoops. Donethanks!

---

src/components/metadata-editor.vue line 470 at r3 (raw file):

Previously, yileifeng (Yi Lei Feng) wrote…

Change to const maxLength = frSlides.length > enSlides.length ? frSlides.length : enSlides.length. If there are TS complaints, can default the eng and fr slides to [].

Donethanks!

---

src/components/metadata-editor.vue line 937 at r3 (raw file):

Previously, yileifeng (Yi Lei Feng) wrote…

Same comment as above, but since this is the same block of logic just throw this in a function to avoid duplicate code

Donethanks!

---

src/components/slide-toc.vue line 271 at r3 (raw file):

Previously, yileifeng (Yi Lei Feng) wrote…

change to bothLanguageSlides.slice(index + 1).every((slide) => slide.fr)

As discussed, probably not necessary here.

---

src/components/slide-toc.vue line 462 at r3 (raw file):

Previously, yileifeng (Yi Lei Feng) wrote…

change to bothLanguageSlides.slice(index + 1).every((slide) => slide.en)

As discussed, probably not necessary here.

---

src/components/slide-toc.vue line 617 at r3 (raw file):

Previously, yileifeng (Yi Lei Feng) wrote…

same suggestion change back to slides

Donethanks!

---

src/components/slide-toc.vue line 648 at r3 (raw file):

Previously, yileifeng (Yi Lei Feng) wrote…

Set this default slide as a local variable to avoid repeating

Donethanks!

[gordlin]

Additional suggestion - perhaps it would be beneficial to move the template code for mobile ToC to its own Vue component if it is feasible to avoid over cluttering one component, will leave that up to you

It's not a lot of additional code, just a few additional styles and lines and whatnot. I think it's fine to leave it.

Reviewable status: 2 of 8 files reviewed, 12 unresolved discussions (waiting on @yileifeng)

[yileifeng]

Tooltips appearing for all panel titles was the default behaviour before too (notice the v-truncate directive from RAMP isn't in this repo). I can implement this if you think it's worthwhile.

Would be good to turn this into a new issue 👍

There are a few issues that pop up now that we're always showing both configs. Because they're saved independently (as they probably should), the ENG/FR config objects are each meant to be a continuous list of existing slides, without random holes. This means there can't really be "isolated" configs.

I see, I think this is something that will come up again for sure since it can look confusing from a user perspective for instance to have the option to delete the last ENG config on slide 12 and the last FR config on slide 14. Intuitively, I think we would want a unified lang slide deletion logic, either having the option to delete any EN/FR slide config individually or not having the option to delete any EN/FR slide config individually at all.

Side note: the warning that pops up for isolated undefined config is very helpful and I did not notice this before 👍

A few basic ideas:

1. Remove the delete last lang config logic, unless there is a specific reason we need this that I missed? Would be the easiest solution and least amount of work overall.
2. Create a placeholder slide in the config if lang is undefined for a slide, then in Storylines code we can have logic to skip past these placeholders but there would no longer be holes when saving. Would need some testing, but initial thought is having an empty {} block would suffice
3. Another low effort option is to default the corresponding lang slide to an empty slide containing the same panel structure as the non-empty lang slide. Probably do not want to go down this route as having an empty/placeholder EN/FR slide is valid.

Would be nice to get some other perspectives from the team on this as well.

Reviewed 1 of 8 files at r1, 5 of 5 files at r4, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions

---

src/components/slide-toc.vue line 271 at r3 (raw file):

Previously, gordlin (Gordon Lin) wrote…

As discussed, probably not necessary here.

👍

[RyanCoulsonCA]

Create a placeholder slide in the config if lang is undefined for a slide, then in Storylines code we can have logic to skip past these placeholders but there would no longer be holes when saving.

This feels like the best way to solve the continuity problem. I can see a lot of future headaches popping up if we try to support a different slide length for each language. Considering the new structure of the table of contents, it makes sense to me that we should have some sort of correlation between the configs, even if it's just setting the config for the missing slide to {} as Yi Lei mentioned.

Reviewed 2 of 8 files at r1, 5 of 5 files at r4.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @gordlin and @yileifeng)

---

src/definitions.ts line 146 at r4 (raw file):

}

export interface SlideForBothLanguages {

Renaming this to Slides for more consistency probably makes sense here.

---

src/components/editor.vue line 458 at r4 (raw file):

            } else {
                const selectedLang =
                    (lang as keyof SlideForBothLanguages) ?? (this.configLang as keyof SlideForBothLanguages);

Correct me if I'm wrong, but I think this can be replaced with the following for one last cleanup step:

const selectedLang = (lang ?? this.configLang) as keyof SlideForBothLanguages

---

src/components/editor.vue line 476 at r4 (raw file):

        this.currentSlide = slideConfig;
        this.slides[this.slideIndex][
            (lang as keyof SlideForBothLanguages) ?? (this.configLang as keyof SlideForBothLanguages)

Same as above, I think this can be replaced with

this.slides[this.slideIndex][(lang ?? this.configLang) as keyof SlideForBothLanguages]

[gordlin]

@yileifeng

Would be good to turn this into a new issue 👍
Donethanks!

@yileifeng @RyanCoulsonCA

I see, I think this is something that will come up again for sure since it can look confusing from a user perspective for instance to have the option to delete the last ENG config on slide 12 and the last FR config on slide 14. Intuitively, I think we would want a unified lang slide deletion logic, either having the option to delete any EN/FR slide config individually or not having the option to delete any EN/FR slide config individually at all...

This feels like the best way to solve the continuity problem. I can see a lot of future headaches popping up if we try to support a different slide length for each language. Considering the new structure of the table of contents, it makes sense to me that we should have some sort of 1-to-1 correlation between the configs, even if it's just setting the config for the missing slide to {} as Yi Lei mentioned.

I tried my best to implement empty slides for saving, but it caused a LOT of problems and new bugs. Instead, I've implemented a slightly different but hopefully still acceptable solution:

The app will accept undefined slides on load, and the user can modify them, replace them with content, etc. However, on save, all undefined slides will be replaced with empty slides (not {}, but a simple blank slide with two empty text panels). The user will be warned if any slides will have to be overwritten like this:

Reviewable status: 4 of 11 files reviewed, 7 unresolved discussions (waiting on @RyanCoulsonCA and @yileifeng)

a discussion (no related file):
Newest changes implement a dropdown options menu for each config, where you can modify them individually: copy the contents of the adjoining-language config (e.g. from "EN" to "FR" of slide 12).

Also redesigned many of the warning modals, as displayed below.

Options menu:

Delete Slide warning:

Copy other config to current config (shown in mobile viewport):

Clear config (replace with blank config):

---

---

src/definitions.ts line 146 at r4 (raw file):

Previously, RyanCoulsonCA (Ryan Coulson) wrote…

Renaming this to Slides for more consistency probably makes sense here.

Donethanks!

---

src/components/editor.vue line 458 at r4 (raw file):

Previously, RyanCoulsonCA (Ryan Coulson) wrote…

Correct me if I'm wrong, but I think this can be replaced with the following for one last cleanup step:

const selectedLang = (lang ?? this.configLang) as keyof SlideForBothLanguages

Donethanks!

---

src/components/editor.vue line 476 at r4 (raw file):

Previously, RyanCoulsonCA (Ryan Coulson) wrote…

Same as above, I think this can be replaced with

this.slides[this.slideIndex][(lang ?? this.configLang) as keyof SlideForBothLanguages]

Donethanks!

[gordlin]

Reviewable status: 3 of 11 files reviewed, 7 unresolved discussions (waiting on @RyanCoulsonCA and @yileifeng)

a discussion (no related file):

Previously, gordlin (Gordon Lin) wrote…

Newest changes implement a dropdown options menu for each config, where you can modify them individually: copy the contents of the adjoining-language config (e.g. from "EN" to "FR" of slide 12).

Also redesigned many of the warning modals, as displayed below.

Options menu:

Delete Slide warning:

Copy other config to current config (shown in mobile viewport):

Clear config (replace with blank config):

Updated Copy other config to current config popup:

---

---

src/definitions.ts line 146 at r4 (raw file):

Previously, gordlin (Gordon Lin) wrote…

Donethanks!

Whoops, this isn't exactly correct. My bad.

I decided not to change the name to Slides because it isn't exactly correct; each SlideForBothLanguages is still one slide, just with multiple configs. Instead I changed it to MultiLanguageSlide, which I think is a decent and descriptive name for what the object represents.

Let me know if there's a better name you'd prefer.

[IshavSohal]

Reviewable status: 3 of 11 files reviewed, 8 unresolved discussions (waiting on @gordlin, @RyanCoulsonCA, and @yileifeng)

a discussion (no related file):
Great work!

I noticed that when clearing the contents of a slide, the contents are still visible within the slide editor and the advanced editor. Only upon switching to a different slide and then switching back does the content get cleared. It would be nice for this slide content to be removed right away. If this isn't a simple fix then we can probably just create a new issue for it once this PR is merged.

---

[gordlin]

Reviewable status: 2 of 12 files reviewed, 9 unresolved discussions (waiting on @RyanCoulsonCA and @yileifeng)

---

a discussion (no related file):
Newest changes modify undefined slide saving behaviour:

• On save, undefined slides will now be converted to empty objects.
• On initial load, empty objects will be converted to undefined slides (for easy handling while within the editor).

A related StoryRAMP PR (ramp4-pcar4/story-ramp#500) prevents crashes related to these empty object slides. For StoryRAMP and editor preview, undefined/empty objects will be ignored.

[gordlin]

Reviewable status: 2 of 12 files reviewed, 9 unresolved discussions (waiting on @IshavSohal, @RyanCoulsonCA, and @yileifeng)

---

a discussion (no related file):

Previously, IshavSohal (Ishav Sohal) wrote…

Great work!

I noticed that when clearing the contents of a slide, the contents are still visible within the slide editor and the advanced editor. Only upon switching to a different slide and then switching back does the content get cleared. It would be nice for this slide content to be removed right away. If this isn't a simple fix then we can probably just create a new issue for it once this PR is merged.

Nice catch. I've fixed this for both copy slide from other lang and clear slide contents. Let me know if I missed anything.

Donethanks!

[yileifeng]

Reviewable status: 2 of 13 files reviewed, 9 unresolved discussions (waiting on @gordlin, @IshavSohal, and @RyanCoulsonCA)

---

a discussion (no related file):

Previously, gordlin (Gordon Lin) wrote…

Newest changes modify undefined slide saving behaviour:

• On save, undefined slides will now be converted to empty objects.
• On initial load, empty objects will be converted to undefined slides (for easy handling while within the editor).

A related StoryRAMP PR (ramp4-pcar4/story-ramp#500) prevents crashes related to these empty object slides. For StoryRAMP and editor preview, undefined/empty objects will be ignored.

A bug with the advanced editor, to reproduce:

1. Make a change (e.g. slide text change)
2. Navigate to a different slide (alternatively, go to preview and notice change is not there)
3. Go back to the original slide and the advanced editor, change is reverted

On the current main build, this is not an issue.

[yileifeng]

Reviewable status: 2 of 13 files reviewed, 9 unresolved discussions (waiting on @gordlin, @IshavSohal, and @RyanCoulsonCA)

---

a discussion (no related file):

Previously, yileifeng (Yi Lei Feng) wrote…

A bug with the advanced editor, to reproduce:

1. Make a change (e.g. slide text change)
2. Navigate to a different slide (alternatively, go to preview and notice change is not there)
3. Go back to the original slide and the advanced editor, change is reverted

On the current main build, this is not an issue.

One additional case: If the last slide in the product contains an empty placeholder config for one of the languages, creating a new slide will initialize with a placeholder for that language's config. Is this intentional?

If the slides with placeholders are not the last slide, creating a new slide will just initialize with default configurations for both languages.

[gordlin]

Reviewable status: 2 of 13 files reviewed, 9 unresolved discussions (waiting on @IshavSohal, @RyanCoulsonCA, and @yileifeng)

---

a discussion (no related file):

A bug with the advanced editor, to reproduce:

1. Make a change (e.g. slide text change)
2. Navigate to a different slide (alternatively, go to preview and notice change is not there)
3. Go back to the original slide and the advanced editor, change is reverted

On the current main build, this is not an issue.

Donethanks!

One additional case: If the last slide in the product contains an empty placeholder config for one of the languages, creating a new slide will initialize with a placeholder for that language's config. Is this intentional?

That was intentional before, but with the new changes in handling undefined configs it's no longer necessary. I've refactored the code so that new slides always include two blank (defined) configs. In addition, I've changed undefined slides so the new blank slide/copy from other config buttons are always visible even if it's not the first undefined slide (e.g. slide 20 FR in your image above would not have its buttons grayed out).

Donethanks!

[IshavSohal]

Reviewed 1 of 8 files at r1, 1 of 5 files at r4, 2 of 7 files at r5, 1 of 6 files at r6, 2 of 6 files at r7, 4 of 5 files at r8, 2 of 2 files at r9, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @RyanCoulsonCA and @yileifeng)

[RyanCoulsonCA]

Reviewed 2 of 7 files at r5, 1 of 6 files at r6, 2 of 6 files at r7, 4 of 5 files at r8, 2 of 2 files at r9, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @IshavSohal and @yileifeng)

[szczz]

Reviewed 1 of 8 files at r1, 1 of 5 files at r4, 2 of 7 files at r5, 1 of 6 files at r6, 2 of 6 files at r7, 4 of 5 files at r8, 2 of 2 files at r9, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @IshavSohal and @yileifeng)

[yileifeng]

Reviewed 2 of 7 files at r5, 1 of 6 files at r6, 2 of 6 files at r7, 4 of 5 files at r8, 2 of 2 files at r9, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @IshavSohal, @RyanCoulsonCA, and @szczz)

[yileifeng]

Reviewed 4 of 4 files at r10, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @IshavSohal, @RyanCoulsonCA, and @szczz)