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

Remove redundant schema file, Remove exclusive editor prop from schema #532

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

Conversation

IshavSohal
Copy link
Member

@IshavSohal IshavSohal commented Feb 10, 2025

Related Item(s)

#491

Changes

  • Removed the redundant StorylinesSchema.json file, since it was not being used anywhere else
  • Prevented the slide-editor component from adding the following props to a panel
    • modified
    • rightOnly
    • centerPanel
    • centerSlide
  • Prevented the image-editor component from adding the id prop to ImagePanel's within a slideshow
  • Removed the above props and userCreated from the schema
  • Called saveChanges() before executing main logic of panelModified() method, so that unsaved changes are captured and used when comparing the provided panel with its corresponding default config
  • Fixed a few TS errors
  • Fixed map src values for product in public folder (was getting errors when testing)

Note

  • Now that the centerPanel and centerSlide properties are not added to panels, the centerPanel and centerSlide props, within the slide-editor, are set by checking the customStyles prop of the panel
    • If both panels have text-align: center; set, then centerPanel is set to true, otherwise false
    • If left panel has text-align: right; set and right panel has text-align: left set , then centerSlide is set to true, otherwise false
  • However, when there is a fullscreen panel, both the center slide content control and the center panel control perform the exact same action, which is to set text-align: center; on the individual panel
    • Within onSlideChanges() it is impossible to distinguish between whether the fullscreen panel has had the center slide content control enabled or the center panel content control enabled
    • So, I decided to disable the center slide content control when there is a fullscreen panel, and to set centerPanel to true when the individual panel has text-align: center; set
    • Let me know if there are any issues with this design decision
  • I noticed that, for map panels, then timeSlider prop exists in the editors schema but not in the storylines schema. However it seems like the timeSlider prop is used within storylines. Should this be added to the storylines schema, or removed from the editor schema?

Testing

Steps:

  1. Create a new product
  2. Create a new slide
  3. Change the type of a panel to a media panel (ex. Image, Video, Chart)
  4. Notice there is no warning modal
  5. Add some content to the panel (without saving)
  6. Switch the type to another type, and observe the warning modal
  7. Toggle the 'fullscreen panel' control, and go to the advanced editor. Observe that the 'rightOnly' prop is not added to the panel
  8. Toggle the 'center panel content' control, and go to the advanced editor. Observe that the 'centerPanel' prop is not added to the panel
  9. Toggle the 'center slide content' control, and go to the advanced editor. Observe that the 'centerSlide' prop is not added to the panel
  10. Create an image panel, and upload more than one image
  11. Go to the advanced editor, and observe that the id prop was not added to any of the image panels

This change is Reviewable

@IshavSohal IshavSohal added the PR: Frontend PR that primarily involves frontend changes. UI experts and CSS Wizards are asked to review. label Feb 10, 2025
Copy link

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

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 3 of 9 files at r1, all commit messages.
Reviewable status: 3 of 9 files reviewed, 2 unresolved discussions


public/StorylinesSlideSchema.json line 414 at r1 (raw file):

                    "description": "A caption to display below the slideshow."
                },
                "userCreated": {

This looks like another temporary schema property exclusive to RESPECT that should be discarded upon saving


public/StorylinesSlideSchema.json line 516 at r1 (raw file):

            "default": true
        },
        "rightOnly": {

Same with "rightOnly", "centerSlide", "centerPanel" which are only used in slide editor formatting

@IshavSohal IshavSohal force-pushed the issue-491 branch 3 times, most recently from b3699aa to 4d0a272 Compare February 14, 2025 19:22
@IshavSohal IshavSohal changed the title Remove redundant schema file, Remove 'modified' prop from schema Remove redundant schema file, Remove exclusive editor prop from schema Feb 14, 2025
Copy link
Member Author

@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: 1 of 13 files reviewed, 3 unresolved discussions (waiting on @yileifeng)


public/StorylinesSlideSchema.json line 414 at r1 (raw file):

Previously, yileifeng (Yi Lei Feng) wrote…

This looks like another temporary schema property exclusive to RESPECT that should be discarded upon saving

Donethanks


public/StorylinesSlideSchema.json line 516 at r1 (raw file):

Previously, yileifeng (Yi Lei Feng) wrote…

Same with "rightOnly", "centerSlide", "centerPanel" which are only used in slide editor formatting

Donethanks


a discussion (no related file):
Now that the centerPanel and centerSlide properties are not added to panels, the centerPanel and centerSlide props, within the slide-editor, are set by checking the customStyles prop of the panel

  • If both panels have text-align: center; set, then centerPanel is set to true, otherwise false
  • If left panel has text-align: right; set and right panel has text-align: left set , then centerSlide is set to true, otherwise false

However, when there is a fullscreen panel, both the center slide content control and the center panel control perform the exact same action, which is to set text-align: center; on the individual panel

  • Within onSlideChanges() there is no way to distinguish between whether the fullscreen panel has had the center slide content control enabled or the center panel content control enabled
  • So, I decided to disable the center slide content control when there is a fullscreen panel, and to set centerPanel to true when the individual panel has text-align: center; set

Let me know your thoughts on this design decision, and if you have any suggestions.

The main issue I have with this is that if the user enables the center slide content control, and then enables the fullscreen panel control, and then disables the fullscreen panel control, the center panel content control would be enabled, as opposed to the center slide content control. If the user wants the center slide content control to be enabled the entire time, they would have to manually set it again.

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.

2 participants