Get multi-sequences to work for v3 ranges/seqs #3333

MImranAsghar · 2020-11-02T19:50:06Z

Update sequences and canvases selectors to get multi-sequences to work for v3 top-level ranges with behavior sequence
Add integration test for v3 multi sequences

mejackreed · 2020-11-03T18:23:48Z

Thanks for moving this forward @MImranAsghar . I'm seeing some issues here which seems related. Can you investigate? https://deploy-preview-3333--mirador-dev.netlify.app/__tests__/integration/mirador/toc.html

MImranAsghar · 2020-11-03T19:00:41Z

Thanks Jack. Yes, looking into it.

codecov-io · 2020-11-10T02:52:41Z

Codecov Report

Merging #3333 (d7f1eea) into master (8612748) will decrease coverage by 0.27%.
The diff coverage is 51.61%.

@@            Coverage Diff             @@
##           master    #3333      +/-   ##
==========================================
- Coverage   89.17%   88.89%   -0.28%     
==========================================
  Files         196      195       -1     
  Lines        3380     3396      +16     
==========================================
+ Hits         3014     3019       +5     
- Misses        366      377      +11

Impacted Files	Coverage Δ
src/state/selectors/sequences.js	`80.39% <41.66%> (-12.11%)`	⬇️
src/state/selectors/canvases.js	`86.11% <52.94%> (-10.50%)`	⬇️
src/components/WindowSideBarCanvasPanel.js	`82.35% <100.00%> (+1.10%)`	⬆️
src/config/settings.js	`0.00% <0.00%> (ø)`
src/components/SearchHit.js	`100.00% <0.00%> (ø)`
src/components/SearchPanel.js	`100.00% <0.00%> (ø)`
src/components/CanvasLayers.js	`100.00% <0.00%> (ø)`
src/components/ManifestForm.js	`84.61% <0.00%> (ø)`
src/components/WorkspaceAdd.js	`96.55% <0.00%> (ø)`
src/components/IIIFThumbnail.js	`90.00% <0.00%> (ø)`
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8612748...62fea91. Read the comment docs.

- Update sequences and canvases selectors to get multi-sequences to work for v3 top-level ranges with behavior sequence, check if full canvases exist in each sequence - Add integration test for v3 multi sequences

... all items including ToC for v3, other changes - Display an option in multi seq select bar that displays all the canvases in v3 sequence along with ToC - Constrain the size of multi seqs select bar - Change sequences test to expect uuid instead of undefined id for first v3 sequence - Add checkes in getSequences selector before setting id for first v3 sequence which has undefined id initially

MImranAsghar · 2020-11-10T18:24:42Z

Hi @mejackreed , i have added some more changes. I think for the tests ● how plugins relate to state › plugin can catch core action in its reducer is the one that is timing out. any suggestions of why this could be? <-- actually i think i know why

MImranAsghar · 2020-11-10T21:23:23Z

when running these tests on mirador multiple times, some tests are giving timeout in a particular run but when I run the tests again the timeout disappears. Is it okay if those tests fail(timeout) in a pull request in that case?

mejackreed

Thanks for continuing to move this forward @MImranAsghar . However, I'm seeing some issues with previous working content: https://iiif.bodleian.ox.ac.uk/iiif/manifest/e32a277e-91e2-4a6d-8ba6-cc4bad230410.json

It would also be helpful I think to either (or both) add some additional unit tests in the modified selectors to better codify the changes that are happening. I'm still not fully clear on what the modeling changes are intended to represent.

mejackreed · 2020-11-17T15:55:48Z

src/containers/WindowSideBarCanvasPanel.js

  label: {
    paddingLeft: theme.spacing(1),
  },
  select: {
+    '& .MuiSelect-icon': {


I'm not sure this works with our themed approach. Can you say more about the need to modify the color here?

Hi Jack, this is for constraining the size of the select dropdown so it does not extend beyond the sidebar panel when there is a long sequence label.. the reasoning for setting background color to white was to make sure that the 'Arrow icon' in the select bar and the long sequence label don't overlap.

mejackreed · 2020-11-17T15:58:19Z

src/state/selectors/canvases.js

    if (!sequence || !window) return undefined;

-    if (!window.canvasId) return sequence.getCanvasByIndex(0);
+    if (!window.canvasId && (typeof sequence.getCanvasByIndex == 'function')) {


Is the case here that the sequence is not a manifesto Sequence object?

Hi Jack, this is for when the sequence represents a 'Range' manifesto object here which does not provide the 'getCanvasById' or 'getCanvases' function. 'Range' manifesto object has 'items' where it lists all the canvases.

mejackreed · 2020-11-17T16:03:18Z

src/state/selectors/sequences.js

+    // v3 sequence (not range sequence): assign id if not there
+    const manifestSequences = manifest.getSequences();
+    if (v3RangeSequences && manifestSequences && manifestSequences.length > 0
+       && !manifestSequences[0].id) {


Can you say more about the case where an id would not be present?

For a v3 range, manifest.getSequences() always seems to return a sequence manifesto object with all the items/manifesto canvases listed by default, even if it is not listed as range sequence. I thought displaying this as an option in the select dropdown (in addition to ranges with sequence behavior) would allow users to view all the canvases and, more importantly, the top-level Table of Contents can be displayed when this option is selected. This sequence object has the id as undefined because manifesto tries to get it from the 'items' property.

color, update getCurrentCanvas

MImranAsghar · 2020-11-23T19:45:52Z

src/state/selectors/sequences.js

+      if (v3RangeSequences.length > 0 && manifest.items && manifest.items.length > 0
+          && manifest.items[0].items && manifest.items[0].items.length > 0) {
+        /** Use manifesto canvases provided in manifest 'items' property to populate
+         * range 'items'/canvases
+        */
+        const canvases = manifest.items[0].items;
+
+        v3RangeSequences.map((sequence) => {
+          if (sequence.items.length === 0) {
+            const updatedSequence = sequence;
+            updatedSequence.items = sequence.canvases.map(canvasId => {
+              const fullCanvas = canvases.find(c => c.id === canvasId);
+              return fullCanvas;
+            });
+            return updatedSequence;
+          }
+          return sequence;
+        });
+      }
+    }


Hi @mejackreed , I made a pull request to 'get' canvases in the iiif manifesto library as well so this specific change won't be needed in future and i will remove it later after that pull request gets accepted. The manifesto pull request is here IIIF-Commons/manifesto#79. Also I was wondering if you got a chance to get feedback from the designer on whether the additional sequence (which includes all manifest canvases) can be created and displayed (even if its not listed as seqs). I can make the changes accordingly after feedback from the designer.

... canvas based on the sequence/range

MImranAsghar force-pushed the multi-sequences-v3 branch from a6cde22 to d7f1eea Compare November 2, 2020 19:57

MImranAsghar force-pushed the multi-sequences-v3 branch from d7f1eea to 2f0fdb5 Compare November 10, 2020 02:52

MImranAsghar force-pushed the multi-sequences-v3 branch from 2f0fdb5 to 1638681 Compare November 10, 2020 03:16

Get multi-sequences to work for v3 ranges/seqs

1314685

- Update sequences and canvases selectors to get multi-sequences to work for v3 top-level ranges with behavior sequence, check if full canvases exist in each sequence - Add integration test for v3 multi sequences

MImranAsghar force-pushed the multi-sequences-v3 branch from 1638681 to d41322c Compare November 10, 2020 03:25

MImranAsghar force-pushed the multi-sequences-v3 branch from d41322c to c97d5da Compare November 10, 2020 17:43

mejackreed requested changes Nov 17, 2020

View reviewed changes

Update seq select icon background ...

f92b5dd

color, update getCurrentCanvas

MImranAsghar force-pushed the multi-sequences-v3 branch from 62fea91 to f92b5dd Compare November 19, 2020 11:15

MImranAsghar commented Nov 23, 2020

View reviewed changes

Update startCanvas sagas to get start

fd4a8e1

... canvas based on the sequence/range

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Get multi-sequences to work for v3 ranges/seqs #3333

Get multi-sequences to work for v3 ranges/seqs #3333

MImranAsghar commented Nov 2, 2020

mejackreed commented Nov 3, 2020

MImranAsghar commented Nov 3, 2020

codecov-io commented Nov 10, 2020 •

edited

MImranAsghar commented Nov 10, 2020 •

edited

MImranAsghar commented Nov 10, 2020

mejackreed left a comment

mejackreed Nov 17, 2020

MImranAsghar Nov 17, 2020 •

edited

mejackreed Nov 17, 2020

MImranAsghar Nov 17, 2020

mejackreed Nov 17, 2020

MImranAsghar Nov 17, 2020 •

edited

MImranAsghar Nov 23, 2020

Get multi-sequences to work for v3 ranges/seqs #3333

Are you sure you want to change the base?

Get multi-sequences to work for v3 ranges/seqs #3333

Conversation

MImranAsghar commented Nov 2, 2020

mejackreed commented Nov 3, 2020

MImranAsghar commented Nov 3, 2020

codecov-io commented Nov 10, 2020 • edited

Codecov Report

MImranAsghar commented Nov 10, 2020 • edited

MImranAsghar commented Nov 10, 2020

mejackreed left a comment

Choose a reason for hiding this comment

mejackreed Nov 17, 2020

Choose a reason for hiding this comment

MImranAsghar Nov 17, 2020 • edited

Choose a reason for hiding this comment

mejackreed Nov 17, 2020

Choose a reason for hiding this comment

MImranAsghar Nov 17, 2020

Choose a reason for hiding this comment

mejackreed Nov 17, 2020

Choose a reason for hiding this comment

MImranAsghar Nov 17, 2020 • edited

Choose a reason for hiding this comment

MImranAsghar Nov 23, 2020

Choose a reason for hiding this comment

codecov-io commented Nov 10, 2020 •

edited

MImranAsghar commented Nov 10, 2020 •

edited

MImranAsghar Nov 17, 2020 •

edited

MImranAsghar Nov 17, 2020 •

edited