Summary

[Jason-Benson]

Updated Helper.ts to manage Summary and Description individually, and correctly label Summary.

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

[demiankatz]

Thanks, @Jason-Benson! See below for some questions and ideas.

[demiankatz]

Thanks, @Jason-Benson -- see below for my thoughts on this. I think it's possible that your existing technical approach can be flipped around a little bit to solve the problem in a way that's a little more bullet-proof.

[demiankatz]

Thanks, @Jason-Benson -- see below for some proposed adjustments!

[demiankatz]

Thanks, @Jason-Benson, this looks pretty reasonable to me now, but I won't merge until others have had a chance to offer feedback.

Also, one question that's still open is whether we want to put a deprecation annotation on the getDescription method, if we're favoring getSummary now.

[jamesmisson]

Hi @Jason-Benson , thanks for your work on this! I've pulled this down and tried it with a few manifests and it works as intended. Is it worth adding a test and fixtures to test the intended results for both getSummary and getMetadata?

National Library of Wales manifests have dual-language descriptions so could be a good fixture to use: https://damsssl.llgc.org.uk/iiif/2.0/2373813/manifest.json

[demiankatz]

Thanks, @Jason-Benson. Not sure if you have further work planned in the queue, but in the meantime, a couple quick suggestions:

1.) We should add some test coverage for getMetadata(). Specifically, we should test the deduplication functionality, to ensure that the description/summary is added when it is not a duplicate of any existing metadata fields, and that it is not added when it is a duplicate. Maybe it would make sense to adjust the bride.json example so that it has a copy of the summary in the metadata as a description, for example. (Or maybe there's an existing test fixture that already follows this pattern).

2.) Maybe it's worth adding a comment to explain what the bride manifest represents (i.e. why do we need this fixture? What cases does it cover? For example, I see that it's a v3 manifest, but does it have any features that make it necessary beyond the existing v3 fixtures? -- I imagine it probably does, but documenting them could prove helpful).

[demiankatz]

Thanks, @Jason-Benson! See below for a few more ideas.

[demiankatz]

Thanks, @Jason-Benson, I think this looks great. See below for some minor simplifications and cleanup.

Beyond that, the other thing I would request is that you create a new branch based off of dev, apply the test style fix there, and open a separate PR. That way, we can merge the test cleanup separately from your work here, and reduce the number of potentially confusing diffs in this PR.

[demiankatz]

Thanks again, @Jason-Benson, this looks reasonable to me now, and the build is passing. I'll hold off on merging until @jamesmisson returns to the office and has a chance to give this one more look.

[jamesmisson]

Hi @Jason-Benson , this looks great, thanks very much for your work! This can be merged as far as I'm concerned.