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

Use start canvas and time props in Ramp for sections and custom start times #5544

Merged
merged 1 commit into from
Jan 2, 2024

Conversation

Dananji
Copy link
Contributor

@Dananji Dananji commented Dec 22, 2023

@Dananji Dananji force-pushed the ramp-start-canvas-time branch from b62d56d to df12d46 Compare December 22, 2023 18:46
Copy link
Contributor

@masaball masaball left a comment

Choose a reason for hiding this comment

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

These changes look good!

We currently have similar behavior to the sections url path with Playlists where ?position= selects a specific playlist item. Should we create a new ticket to add similar url processing to the playlist endpoint to carry over that feature or would it be better to include that work in this PR?

@Dananji
Copy link
Contributor Author

Dananji commented Jan 2, 2024

Right! How does the user gets to that URL in UI? It seems they are both similar use-cases, so I don't mind including that work in this PR.

@masaball
Copy link
Contributor

masaball commented Jan 2, 2024

In 7.6, the ?position= query is added to the URL when a user selects an item from the "Playlist Items" sidebar on the playlist view page. So the full url becomes "https://example.com/playlists/1?position=1".

Since the items sidebar just triggers canvas switches in the manifest, I don't think the URL is generated by UI actions in 7.7 currently.

@Dananji
Copy link
Contributor Author

Dananji commented Jan 2, 2024

I was looking at the previous playlist page in MCO, and we have the ?position= URL in the playlist item link. But in the new UI its the Canvas URL. So it seems to me there's some things we need to discuss and refine on this implementation. And there's a ticket in the backlog related to this #5514.
I guess the playlist work could be part of that ticket?

@masaball
Copy link
Contributor

masaball commented Jan 2, 2024

Yeah, saving the playlist work for #5514 sounds good. Had missed that in the backlog.

So I think we're good to go with this PR.

@Dananji Dananji merged commit 68175ad into develop Jan 2, 2024
@Dananji Dananji deleted the ramp-start-canvas-time branch January 2, 2024 16:51
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.

2 participants