Skip to content
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

Ensure 'copy slide' always makes new RAMP config #497

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

gordlin
Copy link
Member

@gordlin gordlin commented Dec 20, 2024

Related Item(s)

Issue #454

Changes

  • [Fix] Ensures a new RAMP config file is created every time a slide with a map panel is copied, so there aren't any shared configs between slides.

Notes

It doesn't seem we're currently deleting config files when a map panel-containing slide is deleted. Should we do that?

Testing

Steps:

  1. Go to any product (00000000-0000-0000-0000-000000000000).
  2. Create or go to a slide with a map panel. In the RAMP editor, make a few changes from the default.
  3. Go to the advanced editor and note the filename of the config.
  4. Copy the slide.
  5. Open the new slide, go to the map panel, the select the advanced editor. The config filename should be different from the original slide.
  6. Go back to the copied map panel. In the RAMP editor, check the items you changed in step 2. They should still be changed here, even if you didn't press 'Save changes'.

This change is Reviewable

@gordlin gordlin added the PR: Frontend PR that primarily involves frontend changes. UI experts and CSS Wizards are asked to review. label Dec 20, 2024
Copy link

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

Copy link
Member

@IshavSohal IshavSohal left a 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 2 files reviewed, 1 unresolved discussion (waiting on @gordlin)


a discussion (no related file):
The fix works for map panels, but the issue still occurs when the maps are within a slideshow panel.

Not exactly related, but I noticed that an issue similar to #454 occurs for assets and charts. For those, I think you can increment the source count of each asset/chart of the copied slide within copySlide(), rather than create new files.

Regarding deleting the config files, there's a fix for that in #343

Copy link
Member

@RyanCoulsonCA RyanCoulsonCA left a 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 2 files reviewed, 2 unresolved discussions (waiting on @gordlin and @IshavSohal)


src/components/slide-toc.vue line 661 at r1 (raw file):

        // Nested setTimeout calls may not be the best option. It works though.
        // Maybe try replacing with promise chain?
        setTimeout(async () => {

I'd say we should go with Promises instead of the nested timeouts. While it's likely everything will be done before the timeout triggers, we can't guarantee it and it can lead to some hard-to-find bugs later on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Frontend PR that primarily involves frontend changes. UI experts and CSS Wizards are asked to review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants