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

Dynamic Slideshow #353

Closed
wants to merge 1 commit into from
Closed

Dynamic Slideshow #353

wants to merge 1 commit into from

Conversation

mohsin-r
Copy link
Member

@mohsin-r mohsin-r commented May 23, 2023

Closes #318.

You can now add a slideshow that supports all panel types (text, image, chart, map).

⚠️ Caution: This PR makes breaking changes to the current schema, so people should be comfortable with the design before this is merged. The details are in the files, but here are the changes that would be needed to migrate current products:

  • For all the current slideshow panels, just change the type to image.
  • For all the current image panels, add an images array and drop your single image config inside that array.

Essentially, the current image panels now support multiple images (similar to chart panels) and the slideshow panels support all panel types instead of just images.

Feel free to play around with creating, editing, viewing etc. slideshows and suggest changes to design, implementation, etc.


This change is Reviewable

@github-actions
Copy link

Your demo site is ready! 🚀 Visit it here: https://ramp4-pcar4.github.io/story-ramp/318/#/en/00000000-0000-0000-0000-000000000000

Copy link
Member

@spencerwahl spencerwahl left a comment

Choose a reason for hiding this comment

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

I'd post in teams with the required config changes to see how Dan feels about them. They make sense to me but there are a significant number of existing sites.

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

Copy link
Member

@spencerwahl spencerwahl left a comment

Choose a reason for hiding this comment

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

Hm, actually does it make more sense to remove "slideshow" functionality from the images and charts panels and just use the slideshow panel for that, keep the other two for single instances of their types.

Reviewable status: 0 of 6 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.

Nice work on the feature, looks great. One major issue I found when testing was the new dynamic slideshow layout on mobile. With the current sample, we have 2 main UI bugs:

  • the long text panel section of the slideshow hides the second text panel ("This is a dynamic slideshow!...")
  • text overlaps with slide selector:

image.png

Another UI bug with the new image panel with full screen button positioning:

image copy 1.png

Reviewed 5 of 6 files at r1, all commit messages.
Reviewable status: 5 of 6 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.

Hm, actually does it make more sense to remove "slideshow" functionality from the images and charts panels and just use the slideshow panel for that, keep the other two for single instances of their types.

+1 for this if we are holding off on merging until post-release, otherwise current format probably minimizes number of config changes required to existing products.

Reviewable status: 5 of 6 files reviewed, all discussions resolved

Copy link
Member Author

@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 did bring up the config changes to Dan and he said we'd discuss them at the next meeting (likely next week). Will work on the UI fixes.

Reviewable status: 5 of 6 files reviewed, all discussions resolved (waiting on @yileifeng)

Copy link
Member Author

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

Fixed the UI bugs above.

Reviewable status: 4 of 7 files reviewed, all discussions resolved (waiting on @yileifeng)

Copy link
Member

@spencerwahl spencerwahl left a comment

Choose a reason for hiding this comment

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

UX concern; panning a map within a dynamic slideshow (with mouse or on mobile) causes both panning and moving the slideshow. I'm not quite sure what the best solution is but its not a very pleasant user experience.

Reviewable status: 4 of 7 files reviewed, all discussions resolved (waiting on @yileifeng)

Copy link
Member Author

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

This is a very good point, perhaps we consult the UI/UX team? I have a few ideas but they're not great.

  • We can disable dragging the slides to change and have the user just use the navigation/pagination. However, I'm not sure if that's acceptable on mobile view. Maybe for mobile, you need two fingers to pan and one finger to switch slides or vice versa?
  • Maybe we can add some padding around the map. If the user drags on the map, only pan the map. If the user drags on the padded area, do the slide switching. However, with the space on mobile already so limited, adding padding may not be a good idea.

Reviewable status: 4 of 7 files reviewed, all discussions resolved (waiting on @yileifeng)

@AleksueiR
Copy link
Member

The overall idea is to make things easier (and consistent since being consistent makes it easier) for the user.

  1. More consistent map navigation:
    • I think we should treat a map inside a slideshow similarly to a regular embedded map elsewhere in the Storylines app
    • Use the ScrollGuard feature from RAMP4 to only let pan/zoom an embedded Storylines map while holding "Ctrl" on desktop or with two fingers when on mobile. The page shouldn't be scrolled when the map is panned or zoomed.
    • Make sure this behaviour is consistent with a map inside a slideshow and a standalone map component.
  2. Better slideshow:
    • Right now, when you scroll over a map in the slideshow, it moves to the next slide. We want it to work like other things you scroll through, so scrolling over the map makes the slideshow move forward.
    • If that's too tricky, we can make it so scrolling over the map (or any other slideshow element) doesn't affect the slideshow at all. Whichever solution is picked, be consistent.
  3. Slideshow buttons:
    • Right now, the buttons that let you go back and forth in the slideshow jump up and down when the slides are different sizes, and that's confusing. The button height should match the height of the slideshow component with the icon vertically centered, so even though the icon will move up or down when switching between slides of different height, you can still click the next/previous button without moving the cursor.
    • Add a hover effect on the back/forward buttons so the users knows they can still click them even though the button label had moved.
    • It would be cool if the slideshow animated changes in its height as you go through it. This could make the changes between slides look smoother and remove the annoying jump of the next/previous button icons.

@dan-bowerman
Copy link
Member

Recommend we take Aleksuei's UX suggestions and spawn off new issues for those, in the interest of getting this PR pushed through.

Perhaps maps in a dynamic slideshow (a use case we haven't run into yet) can have scrollguard enabled by default?

@dan-bowerman
Copy link
Member

It may be easier to start from scratch on this one, given the work involved in rebasing this to Vue3.

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.

Dynamic gallery - A gallery that supports all panel types
5 participants