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

Additional TOC implementations #2944

Merged
merged 21 commits into from
Mar 5, 2020
Merged

Conversation

lutzhelm
Copy link
Contributor

Changes / Additions:

  • Set the window's current Canvas not only for leaf nodes but for any nodes that contain canvases
  • Track open TOC nodes in the application state in order to be able to reopen them if companion window is moved
  • Visibly mark all ranges that contain currently visible canvases, as well as their parents
  • Open any branch nodes that (or whose children) contain a currently visible Canvas (or part of a Canvas), except if a node has been manually closed
  • Scroll to the first node that contains a currently visible canvas
  • Keyboard navigation (up/down handled by Material-UI TreeView, left/right/enter/space handled by Mirador). This should be replaced by using onNodeSelect once [Treeview] Add node selection support mui/material-ui#18357 is merged.

To be done:

  • Check if Prezi 3 Range items of type SpecificResource (instead of Canvas with or without fragment) do work; support those if that's not the case
  • Styling
  • Tests

I temporarily stored the Range ids of open nodes instead of the node ids themselves but encountered manifests where a Range would appear on different branches of the tree, so I dismissed that idea. Any thoughts on that?

@codecov-io
Copy link

codecov-io commented Mar 2, 2020

Codecov Report

Merging #2944 into toc will increase coverage by 0.7%.
The diff coverage is 97.14%.

Impacted file tree graph

@@            Coverage Diff            @@
##              toc    #2944     +/-   ##
=========================================
+ Coverage   92.58%   93.28%   +0.7%     
=========================================
  Files         158      159      +1     
  Lines        2347     2414     +67     
=========================================
+ Hits         2173     2252     +79     
+ Misses        174      162     -12
Impacted Files Coverage Δ
src/state/actions/action-types.js 100% <ø> (ø) ⬆️
src/state/reducers/companionWindows.js 100% <100%> (ø) ⬆️
src/state/actions/companionWindow.js 66.66% <100%> (+16.66%) ⬆️
src/components/SidebarIndexTableOfContents.js 100% <100%> (+100%) ⬆️
src/state/selectors/ranges.js 95.83% <95.83%> (ø)
src/components/WindowSideBarCanvasPanel.js 72.22% <0%> (ø) ⬆️
src/state/selectors/manifests.js 98.09% <0%> (+1.9%) ⬆️

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 91247a9...8950dce. Read the comment docs.

@@ -57,3 +57,18 @@ export function removeCompanionWindow(windowId, id) {
windowId,
};
}

/** */
export function toggleNode(windowId, id, nodeId) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we add a test for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I hope to get it done by tomorrow.

@@ -25,6 +25,14 @@ export function companionWindowsReducer(state = {}, action) {
return removeIn(state, [action.id]);
case ActionTypes.IMPORT_MIRADOR_STATE:
return action.state.companionWindows;
case ActionTypes.TOGGLE_TOC_NODE:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it possible to add a test for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I vastly simplified the reducer and added a test.

Copy link
Collaborator

@mejackreed mejackreed left a comment

Choose a reason for hiding this comment

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

I noticed one other odd thing happening, have you seen this yet? It seems like with a long list and then switching to compact view that list is not scrollable. Other than that this is looking really good!

Kapture 2020-03-04 at 12 38 20

@lutzhelm
Copy link
Contributor Author

lutzhelm commented Mar 5, 2020

I noticed one other odd thing happening, have you seen this yet? It seems like with a long list and then switching to compact view that list is not scrollable. Other than that this is looking really good!

I have noticed, but you did already fix this last year in #2921. The fix is just not in the branch.

@mejackreed
Copy link
Collaborator

Ah maybe I did. Thanks for pointing that :)

@mejackreed mejackreed merged commit f473552 into ProjectMirador:toc Mar 5, 2020
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.

3 participants