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

fix advanced editor not saving correctly #317

Merged
merged 1 commit into from
Jun 18, 2024

Conversation

RyanCoulsonCA
Copy link
Member

@RyanCoulsonCA RyanCoulsonCA commented Jun 7, 2024

Related Item(s)

#312

Changes

  • This PR fixes an issue where changes in the advanced editor weren't saving correctly.
  • This PR also removes the redundant save button at the bottom of the advanced editor.

Notes

It doesn't seem like the JSON is being validated when you swap between tabs. I'm going to create a new issue for this.

Testing

Steps:

  1. Go to the Editor
  2. Load a product, click Next, select a slide on the left.
  3. Select the "Advanced" tab and make any change. Ensure that changes are being saved correctly.

This change is Reviewable

Copy link

github-actions bot commented Jun 7, 2024

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

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 2 of 2 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @RyanCoulsonCA)

Copy link
Member

@dnlnashed dnlnashed left a comment

Choose a reason for hiding this comment

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

Works good! Just noticed some things that are probably unrelated to this issue and can be made into a new one if needed

  • When removing a panel using the advanced editor, if you click on the panel side that's been removed the slide editor ends up breaking
  • After using the "Reset Changes" button the button still appears. Clicking it again removes everything from the editor.
  • This one might be intentional, but removing the whole config in the advanced editor does not get saved.

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

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.

Nice finds. I'll spin up a new issue for the first one because I haven't been able to find a great way to fix this one.

I can't seem to replicate the second one. Does the button stick around permanently after you've clicked it the first time? When I click the button, I see that it briefly pops up again after reloading but it disappears faster than I can click it again.

For the last one, I'm not sure how Storylines even reacts to a slide with no panels. It's probably best we don't allow the user to remove the whole config... for now we'll say this is intentional, but maybe we should add a warning message.

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @RyanCoulsonCA)

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.

For the last one, I'm not sure how Storylines even reacts to a slide with no panels. It's probably best we don't allow the user to remove the whole config... for now we'll say this is intentional, but maybe we should add a warning message.

Actually, my other open PR that does JSON validation already warns the user if the config is erased so we don't have to worry about this one.

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @RyanCoulsonCA)

Copy link
Member

@dnlnashed dnlnashed left a comment

Choose a reason for hiding this comment

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

Sounds good 👍 . For the second issue the button sticks around for me
Animation.gif

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @RyanCoulsonCA)

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.

Very interesting. I still can't seem to reproduce this one so I'll spin it into a new issue as well.

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @RyanCoulsonCA)

@dnlnashed dnlnashed merged commit c781483 into ramp4-pcar4:main Jun 18, 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.

3 participants