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

Refactor: Create parsed manifest type and centralize parsing in iiif-parser #226

Closed
3 tasks
cjcolvar opened this issue Sep 1, 2023 · 7 comments
Closed
3 tasks

Comments

@cjcolvar
Copy link
Member

cjcolvar commented Sep 1, 2023

Current Code Structure

Components listen for when the unparsed manifest is set in the state. When that happens they call parseManifest and pass it to methods in iiif-parser and set the result in the state. They focus on just the information they need for themselves.

Proposal

Create a type for the parsed manifest with fields ramp needs. iiif-parser will have a single point of entry for parsing which will return an object of the new type to be stored in the state as one large object. The manifest state can then spread the object and update individual values as individual components need to update the state based upon user interactions.

Goals (break into 2 issues)

  1. Change parsing so it's all in one place
  2. Investigate other state management methods to maintain state across components without triggering unnecessary re-renders

Pros

  • Centralized parsing: one place to find all parsing
  • only call parseManifest one time
  • easier maintenance
  • simpler and more understandable code including tests
  • UI component tests can rely on mock state instead of manifest data
  • Could improve performance

Cons

  • Removes the parsing logic and state from components where it is being used
  • Will take a lot of effort
  • Might not improve performance
  • When is the right time to undertake this?

Investigation/Done Looks Like

  • Deep dive to understand React contexts better including how it technically works and when to use local state vs. central state.
  • Determine how best to use contexts to serve the needs of Ramp.
  • Draw up diagram of component interaction.

Prior Work

Proof of concept PR: #222

@cjcolvar
Copy link
Member Author

cjcolvar commented Sep 1, 2023

Looking at clover they seem to recommend have a top-level fetch and then pass the values as props to "primitive" components (https://github.com/samvera-labs/clover-iiif/blob/main/pages/docs/composing.mdx#L166-L191). For non-primitives like Viewer they pass the manifest url and it fetches and parses the whole thing into a context (https://github.com/samvera-labs/clover-iiif/blob/main/src/components/Viewer/index.tsx#L44C1-L55). Curiously that uses a @iiif/vault which is itself a store which fetches, parses, and normalizes presentation manifests (https://github.com/IIIF-Commons/vault/blob/main/docs/getting-started.md). It currently uses redux but is looking at using zustand(https://github.com/pmndrs/zustand) in version 1 (whenever that happens). These provide a lot of examples that could be useful for better forming a path forward with respect to state management.

@cjcolvar
Copy link
Member Author

Another refactor idea: Use React refs in place of state hooks to avoid the number of rerenders. For example structure navigation clicks cause a rerender of that component.

@Dananji
Copy link
Collaborator

Dananji commented Nov 13, 2023

Another way to manage state optimizing the use of the virtual DOM minimizing re-renders in the whole component tree: https://preactjs.com/blog/introducing-signals/

@joncameron
Copy link
Contributor

joncameron commented Mar 19, 2024

Could be broken into 2 main issues: UI update, and components parsing their own information

  • Update parsing - create central state management object (5 points)
  • Change handling of React hooks in each component that get updated when state changes (Break up into 2 tickets: investigation and work following investigation (list of what's needed))

@Dananji
Copy link
Collaborator

Dananji commented Jun 5, 2024

Explore the option of using iiif-helpers either with manifesto or as a replacement of it for Manifest parsing?

@Dananji Dananji mentioned this issue Jul 24, 2024
7 tasks
@joncameron
Copy link
Contributor

We'll want to update state management; explore options for state updates and possibly restructure how the data is arranged between reading the manifest and having different components parse at different times. After this, we could clean up further code in the companion ticket #593.

@joncameron
Copy link
Contributor

We will be making these changes; #605 and #606 are for the work.

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

No branches or pull requests

3 participants