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

Week of 5/22 changes #46

Merged
merged 9 commits into from
Jun 13, 2023
Merged

Week of 5/22 changes #46

merged 9 commits into from
Jun 13, 2023

Conversation

camdendotlol
Copy link
Collaborator

@camdendotlol camdendotlol commented May 22, 2023

Note

Since Nick is away this week and I have several interconnected tasks, I've decided to make all the changes in one PR. I know it's not the best as far as code review goes, but it's difficult to work across several branches that build upon one another and I know the rebases would be a whole project in themselves.

Summary

  • removes the Introduction story from the Storybook folder so EditionCrafter will pop up right away when you npm run storybook

#13 - Configurable transcription types

  • updates the Navigation component to use the values from the config object when building the transcription type dropdown
  • updates the logic in loadFolio to only resolve the folio object when all transcription types are loaded
    • Because the folio object is being mutated within that function and not being used in e.g. a Redux store or a component's state, the components don't know to re-render when a new transcription type finishes loading. This caused crashes when attempting to switch between types in the viewer. With this change, I've made it watch for all the types to load using a tracker object, and only resolve when they've all finished - the component can then rely on having all the data for all the transcription types. In the long run, it would be a lot better to refactor loadFolio so it writes to a Redux state, so any components that connect to it automatically re-render. But this issue is fixed for now!
  • adds a couple null checks to TranscriptionView to handle when a folio doesn't have a given transcription

#15 - Inline figures

  • updates the htmlToReactParserOptions to handle figure elements from the TEI

This assumes a structure of:

<figure>
	<graphic url="/cats/hungry.png" />
	<figDesc>I'm a description!</figDesc>
</figure>

The figDesc is optional, but if it exists the function will add an alt tag and a figCaption HTML element containing the text.

#14 - Moving comments from JSON file to TEI

  • updates TranscriptionView to display the EditorComment popup when a tei-note element is encountered
  • removes everything related to the old comments.json technique, including the Redux state stuff and loading logic

Note

The note contents are still parsed as HTML using html-react-parser. It may be desirable to simplify this to a simple string display, but I didn't want to assume anything.

#16 - Glossary file in manifest

  • updates the state management and saga logic to use the glossary URL from the manifest, instead of the one passed in via the config object

The changes on the CLI side at cu-mkp/editioncrafter-cli#11 are more substantial and more likely to require changes.

Screenshots

Inline figure

Screenshot 2023-05-23 at 11 48 13 AM

@camdendotlol camdendotlol requested a review from NickLaiacona May 24, 2023 16:40
@NickLaiacona
Copy link
Collaborator

This is great - I tried out #13 with some test data and that worked great for me. On #16, I was hitting an error here switching pages. It was blocking me, so I fixed it with this modification:

function* resolveGlossary(manifest) {
  const glossary = yield select(justGlossary);
  if (!glossary.loaded) {
    if (
      !manifest?.seeAlso
      || manifest.seeAlso.length === 0
      || !manifest.seeAlso[0].id
    ) {
      throw new Error('Missing glossary link in seeAlso array.');
    }
  
    const glossaryURL = manifest.seeAlso[0].id;  
    const response = yield axios.get(glossaryURL);
    yield putResolveAction('GlossaryActions.loadGlossary', response.data);
  }
}

If this looks good, please include it in your PR, I didn't check it in on my end. Also, I think it will be necessary to make the glossary an optional thing, not every project will have one. This is probably a seperate issue/feature.

I haven't looked at #14 and #15 yet but will feedback here when I do.

@NickLaiacona
Copy link
Collaborator

Hi @camdendotlol I looked at this for a bit, it would be a lot easier for me to test if you could patch in the code for the glossary, makes stuff on my branch of editioncrafter-cli work beter.

@camdendotlol
Copy link
Collaborator Author

@NickLaiacona Just seeing this now - I'll set a reminder to do it first thing Monday.

@camdendotlol
Copy link
Collaborator Author

@NickLaiacona Done.

Copy link
Collaborator

@NickLaiacona NickLaiacona left a comment

Choose a reason for hiding this comment

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

completed review of #14 and #15, all looks great!

@camdendotlol camdendotlol merged commit 433e9cb into dev Jun 13, 2023
@camdendotlol camdendotlol mentioned this pull request Jun 13, 2023
@camdendotlol camdendotlol deleted the cm/configurable-t-types branch September 1, 2023 13:29
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