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

implement dynamic slideshow editor #269

Merged
merged 1 commit into from
Apr 5, 2024

Conversation

RyanCoulsonCA
Copy link
Member

@RyanCoulsonCA RyanCoulsonCA commented Mar 21, 2024

Related Item(s)

ramp4-pcar4/story-ramp#410

Changes

  • Adds the dynamic slideshow editor.
  • The existing chart and image editors still support uploading multiple charts and images, but their config is converted to a slideshow behind the scenes for a better user experience. The chart and image editors used in the dynamic slideshow editor only support one image or chart to prevent slideshow-ception.
  • Removed some unused properties frmo definitions.ts

Notes

  • I'm open to any and all UI changes for the dynamic panel. If you have an idea feel free to let me know!

Testing

Steps:

  1. Create a new slide with the type slideshow and play around with it.
  2. The preview button will not currently work because of the schema changes (we need to merge the issue in the story-ramp issue first). You can verify that the changes are working by looking at the config files directly in the meantime!

This change is Reviewable

@RyanCoulsonCA
Copy link
Member Author

Because of the schema changes, existing products being built with RESPECT that contain chart or image panels will break. Do we think it's a good idea to add backwards-compatibility to the editor to automatically convert existing charts/image panels to the new schema?

Pros:

  • Would create less of a headache for the people working on existing products.
  • It shouldn't be that much work to implement.

Cons:

  • It kind of bloats our codebase. We could always remove it in the future though (give the clients a heads up they have x number of weeks/months to make the fixes, for example).
  • Even though it shouldn't be much work, it's more work towards this issue that we've already spent a lot of time on

@RyanCoulsonCA
Copy link
Member Author

@dan-bowerman
Copy link
Member

dan-bowerman commented Mar 21, 2024

Do we think it's a good idea to add backwards-compatibility to the editor to automatically convert existing charts/image panels to the new schema?

The migration from the "Beta Editor" and its existing products will be a one-time event that will be happening hopefully soon. I was actually just talking about this with Mohsin regarding having similar functionality for upgrading RAMP3 map configs. As this functionality would only be needed once, we opted to not pursue it.

What would need to happen to the charts and image panels to make them work? Would perhaps a standalone script for "upgrading" existing Storylines suffice - perhaps just point it at a folder full of Storylines products and run? I don't think it's a good idea to bloat the RESPECT codebase for a one-time event.

@RyanCoulsonCA
Copy link
Member Author

The config changes required are outlined in the PR in the story-ramp repo (issue: ramp4-pcar4/story-ramp#411).

To fix existing chart galleries, change the type from chart to slideshow, change the charts property to items, and add type: "chart" to each of the entries (it now treats each of the charts as their own chart panel).

To fix existing image slideshows, simply replace the images property with items.

These changes are simple enough that a standalone script should get the job done. If we want to bring our existing prod sites up to a new version we would need to run this script on those as configs well, obviously with backups just in case!

Copy link
Member

@mohsin-r mohsin-r left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I attempted to play around with it. I was able to successfully create a slideshow and I like the UI and functionality of it. However, when I tried previewing the product the slideshow didn't display and there are some console errors.
Another interesting point to consider with the slideshow is whether we can simply remove image, chart, and map types. Since the slideshow encompasses all cases of these individual panel types.

Reviewable status: 0 of 11 files reviewed, all discussions resolved

@RyanCoulsonCA RyanCoulsonCA force-pushed the fix-410 branch 2 times, most recently from 9c9074b to f1df870 Compare March 22, 2024 14:49
Copy link
Member Author

@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.

The preview wasn't working because we hadn't updated the NPM package to include the new dynamic slideshow changes in Storylines. I've updated the package now so it should be working! The maps are using a RAMP3 config so won't work until we merge in the RAMP4 editor.

For your second point, if I'm understanding it correctly, my main concern would be that I feel we would be taking a lot away from the user experience aspect. The reason why I added extra logic to keep all-image/all-chart slideshows on their respective editors instead of displaying the slideshow editor is that the user would need to do a lot more button pressing to add new charts, and images would have to be uploaded one at a time instead of just being able to drop a bunch in. Open to hearing the opinions of others on this!

Reviewable status: 0 of 14 files reviewed, all discussions resolved

Copy link
Member

@yileifeng yileifeng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with keeping the individual image/chart/map editors for the flexibility and it does not seem intuitive for users to jump to the slideshow editor for simple panels such as a one image panel.

Reviewable status: 0 of 14 files reviewed, all discussions resolved

Copy link
Member

@yileifeng yileifeng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Problematic behavior appears when there are multiple charts in the dynamic slideshow:

The default file name of each chart is the same for each new chart editor that gets added, and this results in all charts in the slideshow pointing to the same file/instance (as opposed to adding multiple charts in the standalone chart editor which just has a integer incremented). Will need to figure out better default naming structure for unique file names.

In addition, renaming the chart file will cause more issues. This can be turned into a new issue, but we haven't really noticed the chart file naming to be much of an issue until now where it is magnified.

Reviewable status: 0 of 14 files reviewed, all discussions resolved

Copy link
Member Author

@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.

Made some changes:

  • Enhanced our check to see if we're creating a chart with a duplicate name. It will now check to see if the new chart name already exists using the pre-existing sourceCounts object. If the file name is already in use, the user is alerted and the modal re-appears for the user to fix the issue.
  • Instead of hiding the "add new chart" button, it is now disabled. There was an issue with having the v-if check on the same component that we had the modal connected to.
  • Fixed a bug where if you delete a slide while the editor is still open, nothing happens.

Reviewable status: 0 of 15 files reviewed, all discussions resolved

Copy link
Member

@yileifeng yileifeng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Chart issues seem to be resolved 👍

Reviewed 4 of 11 files at r1, 5 of 5 files at r2, 6 of 6 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @RyanCoulsonCA)

Copy link
Member

@KashishMistry KashishMistry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 4 of 11 files at r1, 5 of 5 files at r2, 6 of 6 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @RyanCoulsonCA)

Copy link
Member

@mohsin-r mohsin-r left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 4 of 11 files at r1, 5 of 5 files at r2, 6 of 6 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @RyanCoulsonCA)

Copy link
Member

@szczz szczz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 4 of 11 files at r1, 5 of 5 files at r2, 6 of 6 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @RyanCoulsonCA)

Copy link
Member

@yileifeng yileifeng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 13 of 13 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @RyanCoulsonCA)

@yileifeng yileifeng merged commit 5b18909 into ramp4-pcar4:main Apr 5, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants