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

Parsing refactor #625

Merged
merged 11 commits into from
Sep 4, 2024
Merged

Parsing refactor #625

merged 11 commits into from
Sep 4, 2024

Conversation

Dananji
Copy link
Collaborator

@Dananji Dananji commented Aug 16, 2024

Related issue: #606

Changes:

  • Remove use parseManifest from manifesto.js as much as possible. Instead use JSON blob returned from the fetch request to read and parse relevant data for Ramp components (this is equivalent to loadManifest function in manifesto.js).
  • Move parsing of custom start, canvas information, and rendering files to IIIFPlayerWrapper component as these values are needed in different components. Store this information in state to make available them to child components. This helps to save this information into state before storing manifest, which triggers rendering of child components.
  • Move parsing and storing of auto-advance, isPlaylist, and annotation service information into manifest-context as these can be parsed and stored asynchronously in state to be used later.
  • Parse all resource related information in a Canvas in getMediaInfo function including placeholderCanvas.

Not changed:

  • Use of parseManifest in parsing structures as this makes the parsing of ranges easier than using the JSON object of manifest. And this code is more clear and readable.

Note

I missed the previous work done on the refactor done by @cjcolvar. But we should probably re-visit that when doing the state management work. I think this work is more related to asynchronous state management than parsing, which is one of the things we should probably look at as part of that refactor.

@Dananji Dananji force-pushed the parsing-refactor-606 branch from 5d16a7b to 8268279 Compare August 19, 2024 21:18
@Dananji Dananji changed the title Parsing refactor 606 Parsing refactor Aug 21, 2024
@Dananji Dananji marked this pull request as ready for review August 21, 2024 20:08
@Dananji Dananji force-pushed the parsing-refactor-606 branch from ded5f90 to 9593904 Compare August 23, 2024 14:12
Copy link
Member

@cjcolvar cjcolvar left a comment

Choose a reason for hiding this comment

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

This looks great! Getting rid of most of those parseManifest calls as well as manifesto objects and methods cleans it up a lot and probably will make it faster too!

I had a few small questions/suggestions but none of them are blocking.

src/context/manifest-context.js Outdated Show resolved Hide resolved
src/components/MediaPlayer/MediaPlayer.js Show resolved Hide resolved
src/components/MediaPlayer/MediaPlayer.js Outdated Show resolved Hide resolved
src/services/iiif-parser.js Outdated Show resolved Hide resolved
@Dananji Dananji requested a review from cjcolvar September 4, 2024 14:49
@Dananji Dananji merged commit 453a42c into main Sep 4, 2024
2 checks passed
@Dananji Dananji deleted the parsing-refactor-606 branch September 4, 2024 17:30
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