-
Notifications
You must be signed in to change notification settings - Fork 257
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
Solve vertical display order and Choice behavior, fix #3693 and #3585 #4015
base: main
Are you sure you want to change the base?
Conversation
The manifest you are using would very likely be a case for The current example is displaying the layers in the panel in the correct order, but it is loading them in the viewing area backwards. The proposed change version is loading the images in the viewing area correctly, but they are in reverse order in the layers panel. |
Thank you very much for your input! I can see the rationale for sorting the images in the Layer controls by their significance within the manifest, but I am struggling to change the controls adequately. Both a "Move to top" or a "Move to bottom" button would be ambiguous as they could either refer to the stack on the Canvas or to the list in the layer controls; the effect on the respective other element would always be the opposite of what is actually happening in that element; moving an image to the top of the Canvas moves it to the bottom of the layers panel and vice versa. One option to solve this would be something like "Move to background"; it would require replacing both the icon and all translations. However, it would deviate from the layering behavior e.g. of all image editors I can think of; they usually put the background layer at the bottom of a layers list. Would users understand what is happening? The current example manifest include both variants; it has five image annotations, only one of which is a Choice. The full color default option and the four additional image annotations are loaded and displayed by default - instead of all 23 ( 😮 ) image layers as before. The PR is already adding the image composition cookbook recipe example to the manifest list for the |
Maybe "Move to top of index"? And to your point about layering behaviors, I get what you mean re: editing apps, but the reverse is true in the context of the web, where the background image would appear first in the list (z-index behavior), so it's going to be relative I guess (and most users could likely not have experience with either context). I think I would try to avoid "top" and "bottom" as you suggested - unless you can be specific re: top of "what". I'll give it more thought. I'll add that as a user, I would prefer not having to scroll all the way down in a long list of image layers to see the default |
ea0fc77
to
80435b1
Compare
I changed the behavior of the Layers panel to display the images in the panel in the order in the manifest. Currently, the "move to top" button has been replaced with "move to button". However, I wonder what the most useful option would actually be:
The changes are available here. |
2761828
to
b33ede3
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4015 +/- ##
==========================================
+ Coverage 94.59% 94.60% +0.01%
==========================================
Files 315 315
Lines 14759 14796 +37
Branches 2490 2500 +10
==========================================
+ Hits 13961 13998 +37
Misses 794 794
Partials 4 4 ☔ View full report in Codecov by Sentry. |
This PR demonstrates a way how #3693 and #3585 could be addressed. At the current state, it is more of a hack than an actual solution. The tests are not adjusted yet as I don't expect the behavior to stay like it is.
This addresses two problems:
At first I explored the possibility to solve the first problem via changes to the layers redux state, e.g. by setting all less preferred Choice option images invisible after adding a manifest to a window or after changing the current Canvas, but this would lead to extensive and error prone rework. The current "solution" does not require any changes to the state; instead it changes the default behavior. It is, however, not the cleanest way to do this. One problem is that currently, to my knowledge, manifesto does not indicate in any way if an image annotation is a Choice option, and if so, if it is the preferred one. There is a comment in the code that acknowledges that:
https://github.com/IIIF-Commons/manifesto/blob/63a230dfa0584ed71eeed956e1d0f134ec9b4bed/src/Annotation.ts#L25
We could try to make a suggestion for manifesto, but I'm not sure this will be ready before we want to release M4.
Another issue for discussion is the behavior of the Layer sidebar panel. Should images be ordered like they are in the annotations of the Canvas (current behavior, and also the behavior of Mirador 2), or would it be more comprehensible if they were ordered from top to bottom in the same way that they are ordered on a z-axis on the Canvas (behavior in the PR branch)? The current "Move layer to top" button would also make more sense for the latter behavior. It would have to be reshaped if the order in the panel stays as it is.
I'd appreciate any thoughts both on the layer panel order and on possible technical solutions with or without manifesto changes. I'd also be happy about more examples of v2 and v3 manifests with multiple images, Choices or both.