Skip to content

Conversation

@domkm
Copy link
Contributor

@domkm domkm commented Jan 28, 2024

Implements #372.

@mickael-menu I went with an enum instead of a struct but I can alter it if you wish. Thanks!

@domkm
Copy link
Contributor Author

domkm commented Jan 28, 2024

Should I also remove the private extension on Locator?

Copy link
Member

@mickael-menu mickael-menu left a comment

Choose a reason for hiding this comment

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

Thank you @domkm!

The enum is great. Could you add some documentation for the public APIs, maybe linking to the spec? https://www.w3.org/TR/media-frags/#media-fragment-syntax

You can also remove the private extension and use the new one instead.

Thanks again.

@mickael-menu
Copy link
Member

Ha forgot to ask, could you rebase it to develop instead of main? Thanks.

@domkm domkm changed the base branch from main to develop January 29, 2024 16:47
@domkm
Copy link
Contributor Author

domkm commented Jan 29, 2024

When adding that doc string, I realized that my names are not aligned with the W3 doc that you sent. They use begin, end, and interval, while I used offset, duration, and range. Should I update my names to align with the W3 doc?

@mickael-menu
Copy link
Member

Yes I think that would be best to stick to the standard when possible 👍

@domkm
Copy link
Contributor Author

domkm commented Jan 29, 2024

👍

Copy link
Member

@mickael-menu mickael-menu left a comment

Choose a reason for hiding this comment

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

Thank you for making the changes @domkm!

@mickael-menu mickael-menu merged commit efba3a0 into readium:develop Jan 30, 2024
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