Hide panel change warnings when no changes made

[gordlin]

Related Item(s)

Issue #331

Changes

• [FEATURE] Hide "Panel changes will be deleted" warnings when there have been no changes made to the relevant panel. This affects two app interactions:
  • Changing the panel type via the Content Type dropdown
  • Changing the Make the right panel the full slide checkbox

Testing

Steps:

1. Open any storylines product (I used 00000000-0000-0000-0000-000000000000).
2. Click New Slide to add a new slide. Go to that new slide.
3. Use the Content Type dropdown to change the panel type a few times. As long as you don't add any content, the panel type should change without a pop-up coming up.
4. Change to the slide to the "Text" type. Add something to the panel (e.g. change the panel title). Now try changing the panel type again; the popup should show up now.
5. Change to the right panel and try steps 3-4 again; results should be the same.
6. Click New Slide to add a new slide. Add some content to the right panel, then switch back to the left panel and try changing the panel type to "Dynamic". A warning message should pop up, as changing to dynamic overwrites both panels, not just the one you're on.
7. Click New Slide to add a new slide. Click the Make the right panel the full slide checkbox. As long as no changes are made to the left panel when there are both panels, there should be no pop-up warning.
8. Try adding a change to the left panel and then clicking the checkbox; the pop-up should show up. (Note: The pop-up should now never show up when the left panel is hidden, unless there are changes and you are changing panel types).

---

This change is 

[github-actions]

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

[yileifeng]

Right idea and the approach will work as long as currentSlide is updated, however this is not always the case for multimedia editors especially. For example, if you create a new slide with an image panel and upload an image, changing to a different editor type will no longer give the user the pop-up warning because the currentSlide was never updated (unless you navigate to a different panel or slide). This also applies to map, chart, slideshow, video editors.

A few potential solutions for this is to always dynamically keep the currentSlide up to date whenever user makes content changes (like the text editor does). Alternatively, can attempt to update currentSlide or extract slide config from child editor component when user attempts to switch editor types and use that for panelModified comparison.

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @gordlin)

---

src/components/slide-editor.vue line 441 at r1 (raw file):

                config: `${this.configFileStructure.uuid}/ramp-config/${
                    this.configFileStructure.uuid
                }-map-${this.getNumberOfMaps()}.json`,

Bit confused about the starting map config property, shouldn't this be an empty string? I see for other startingConfig instances across the app this is the case. Similarly, can we just set title and content properties to empty strings for dynamic panel configs above (unless there is a specific use case this breaks)?

On that note, I think it is best to move the startingConfig object to the definitions.ts file and import it in the components where it is needed as there are multiple instances using the same/similar startingConfig object (if there are minor differences can just create a separate local component variable and modify the property needed). If that can be done in this PR would be great code cleanup.

[gordlin]

I've fixed these issues, and also all remaining edge cases (that I know of).

There should now also be no pop-up for these cases:

• Selecting an image in the image editor and then deleting it
• Adding slides in the slide editor and deleting them all
  • Strange case in slideshow editor, where selecting a map slide ensured there would always be a pop-up, even after the above was fixed
• Adding chart in the chart editor and then deleting it
• Modifying any field in the video editor and then erasing all changes

Reviewable status: 0 of 7 files reviewed, 1 unresolved discussion (waiting on @yileifeng)

---

src/components/slide-editor.vue line 441 at r1 (raw file):

Previously, yileifeng (Yi Lei Feng) wrote…

Bit confused about the starting map config property, shouldn't this be an empty string? I see for other startingConfig instances across the app this is the case. Similarly, can we just set title and content properties to empty strings for dynamic panel configs above (unless there is a specific use case this breaks)?

On that note, I think it is best to move the startingConfig object to the definitions.ts file and import it in the components where it is needed as there are multiple instances using the same/similar startingConfig object (if there are minor differences can just create a separate local component variable and modify the property needed). If that can be done in this PR would be great code cleanup.

Refactored.

[gordlin]

Reviewable status: 0 of 7 files reviewed, 1 unresolved discussion (waiting on @yileifeng)

---

src/components/slide-editor.vue line 441 at r1 (raw file):

Previously, gordlin (Gordon Lin) wrote…

Refactored.

Additional note, as I forgot to address the first half: I believe the extra map config stuff is necessary here. When we switch to the map panel type, the createNewConfig() function inside the map editor is called, which changes the config value from a blank string to this. Comparing the panel to one with a blank config value would thus make this checker think there've been changes when there haven't.

There may be a similar issue with the dynamic configs.

[yileifeng]

Behavior is working properly for all cases I tested 👍

Reviewable status: 0 of 7 files reviewed, 6 unresolved discussions (waiting on @gordlin)

---

src/definitions.ts line 287 at r3 (raw file):

}

export const baseStartingConfig: DefaultConfigs = {

capitalize the naming for consistency

---

src/components/slide-editor.vue line 41 at r3 (raw file):

                                    $vfm.open(`right-only-${slideIndex}`);
                                } else {
                                    // currentSlide.panel[panelIndex].modified = false;

Remove commented out line if unneeded

---

src/components/slide-editor.vue line 289 at r3 (raw file):

                    :centerSlide="centerSlide"
                    :dynamicSelected="dynamicSelected"
                    @slide-edit="(args: any) => {

Code block is a bit long so should go inside a new component method. For the parameter, can just use madeChanges: boolean (or some other descriptive name) instead of args: any. Can we set the param default to True to remove the undefined check as this is the default/else behavior? More on that below.

---

src/components/slide-editor.vue line 295 at r3 (raw file):

                            // (different from initial state). Only needed for some multimedia editors; text editors
                            // write directly to currentSlide constantly, which is handled by panelModified().
                            switch(currentSlide.panel[panelIndex].type) {

I think I understand the idea, but it would be much much cleaner code to not add a switch/case statement for a subset of panels. I assume the default case is what gets processed for text and dynamic panels? In that case, maybe defaulting the boolean parameter to True would work. An alternative approach is to pass the panel config as a parameter when emitting and run that against the panelModified helper. Ideally, we just want a general method to normalize behavior of when slides are edited for different panel types and remove the switch statement which should be doable.

---

src/components/slide-editor.vue line 300 at r3 (raw file):

                                case 'video':
                                case 'slideshow':
                                    currentSlide.panel[panelIndex].modified = (args as boolean) ? true : undefined;

Could be misunderstanding, but should this be something like if (args) { currentSlide.panel[panelIndex].modified = args } (with a better variable name for args)?

---

src/components/slide-editor.vue line 326 at r3 (raw file):

            "
            @ok="
                // currentSlide.panel[panelIndex].modified = false;

Remove

[gordlin]

Reviewable status: 0 of 7 files reviewed, 6 unresolved discussions (waiting on @yileifeng)

---

src/definitions.ts line 287 at r3 (raw file):

Previously, yileifeng (Yi Lei Feng) wrote…

capitalize the naming for consistency

Donethanks!

---

src/components/slide-editor.vue line 41 at r3 (raw file):

Previously, yileifeng (Yi Lei Feng) wrote…

Remove commented out line if unneeded

Donethanks!

---

src/components/slide-editor.vue line 289 at r3 (raw file):

Previously, yileifeng (Yi Lei Feng) wrote…

Code block is a bit long so should go inside a new component method. For the parameter, can just use madeChanges: boolean (or some other descriptive name) instead of args: any. Can we set the param default to True to remove the undefined check as this is the default/else behavior? More on that below.

Block's been shortened to one line, so it shouldn't need a new method anymore.

---

src/components/slide-editor.vue line 295 at r3 (raw file):

Previously, yileifeng (Yi Lei Feng) wrote…

I think I understand the idea, but it would be much much cleaner code to not add a switch/case statement for a subset of panels. I assume the default case is what gets processed for text and dynamic panels? In that case, maybe defaulting the boolean parameter to True would work. An alternative approach is to pass the panel config as a parameter when emitting and run that against the panelModified helper. Ideally, we just want a general method to normalize behavior of when slides are edited for different panel types and remove the switch statement which should be doable.

Donethanks, refactored into a single line.

---

src/components/slide-editor.vue line 300 at r3 (raw file):

Previously, yileifeng (Yi Lei Feng) wrote…

Could be misunderstanding, but should this be something like if (args) { currentSlide.panel[panelIndex].modified = args } (with a better variable name for args)?

Setting args/changedFromDefault to false messes with panelModified()'s check whether the panel's different from its default configuration object ({} is different from {property: false}). Setting it to undefined prevents this issue.

I've refactored this bit of code to be cleaner (use || instead of ternary).

---

src/components/slide-editor.vue line 326 at r3 (raw file):

Previously, yileifeng (Yi Lei Feng) wrote…

Remove

Donethanks!

[yileifeng]

Reviewed 3 of 7 files at r2, 4 of 4 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @gordlin)

[IshavSohal]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @gordlin)

a discussion (no related file):
Noticed that when you create a new slide, change the content type to slideshow, add text to the title and then remove it, changing the content type again (to something other than dynamic) brings up the warning.

All other cases appear to be functioning correctly though.

---

[gordlin]

Reviewable status: 5 of 7 files reviewed, 1 unresolved discussion (waiting on @IshavSohal and @yileifeng)

a discussion (no related file):

Previously, IshavSohal (Ishav Sohal) wrote…

Noticed that when you create a new slide, change the content type to slideshow, add text to the title and then remove it, changing the content type again (to something other than dynamic) brings up the warning.

All other cases appear to be functioning correctly though.

Donethanks!

---

[RyanCoulsonCA]

Tried my best to break it but couldn't, nice work!

Reviewed 3 of 7 files at r2, 2 of 4 files at r4, 2 of 2 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @IshavSohal)

[IshavSohal]

Reviewed 3 of 7 files at r2, 2 of 4 files at r4, 2 of 2 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @gordlin)