Share dialogue simplified to readonly inputs#1318
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
See below for one small comment. I haven't read through all of the code changes yet, but a few general thoughts and observations:
1.) Are you confident that it's safe to fully rip out all of the things being ripped out here? For example, are we sure there are no other use of tabs that might rely on the tab styles being removed?
2.) Is it possible that we can/should separate some of the general cleanup work from the functional changes so that the history is easier to read/understand? If all of these things are truly entangled, I can live with it... but this is currently a very large change, which makes it a bit hard to conceptualize.
3.) There is a keyboard problem that impacts both this PR and the current dev branch. If I click into a text box and try to navigate with the keyboard (for example, using Home and End keys to see the beginning or end of the URLs), the left panel takes over control, and my keypresses navigate the thumbnails instead of moving the cursor, even though focus is on the text box. Can we do anything about this? It seems less than ideal. Obviously, it's a separate issue, and should be addressed as such. Depending on what the solution is, though, I wonder if it makes more sense to address as a fix to dev first, or to try to fix after we do this refactor. Let me know if I should file a separate issue to track it!
4.) I wonder if we need a better label than "Share" for the top box. I think "Share" should be the overall dialog box name, but the top link is something like "Link to this Page".
5.) Should we have copy to clipboard buttons for any/all of these? (Maybe that's a separate issue, but it seems like it would be helpful, especially in light of other keyboard weirdness).
6.) This PR seems to lose the drag-and-drop IIIF logo. I think there may be reason to keep that; it just likely needs a better label, and possibly a config setting to enable/disable.
|
Its much clearer where the manifest link is and how to copy it. |
|
I did use the label that was on the tab to label the only box in that tab. I feel strongly that adding tabs back in and having three tabs with one input each is a worst UX solution than presenting them as a list. Yes we can add a copy button. Yes I’ll add the deaf and drop back. Is this what the iframe is for? |
|
@crhallberg, the iframe is for embedding the entire viewer (e.g. in a blog post, etc.). The drag and drop is a separate mechanism; there was a movement a few years ago to add drag and drop support to IIIF viewers so that you could open manifests by just dragging them from one place to another. The use case for this isn't really strong for UV, which is designed for a simple viewing experience... it possibly makes more sense with some of the more scholarship-oriented tools that involve multiple open documents, annotations, etc., where you may have multiple documents open at once and want to move them around. But we might as well continue to support the technical standard, even if it's not our primary use case! |
|
@demiankatz what I mean is that there is an actual iframe embedded in the share dialogue. It is the reason why there is blank space in the screenshot above. I'll dig into it more - but it was one of the things I didn't think was wise to remove without understanding. |
Oh, strange, I don't know why that would be the case! |
|
So. Looking into the code. It seems that the share iframe is supposed to show the manifest in the share menu? As it is, it is too small to do so and also comes up as an invisible white on white square. For now, I will remove the iframe. If we get clarification on what feature the iframe is intended to provide, I will build that bridge ro cross myself. |
No objection here -- I don't see an advantage to embedding the viewer inside its own share dialog. That just seems like a road to madness in the hall of mirrors. |
|
See updated screenshots in description (top). |
|
Would it make sense to checkout/not commit the docs until the end of this process? It seemed to happen automatically when dev updated docs. |
There's a GitHub Action that automatically generates and commits the docs so that we don't forget and let them get out of sync. It's just a bit distracting when you make changes low in the config hierarchy because that ends up impacting a lot of documentation pages. I can't think of a way around this, though, without risking the docs getting out of date again (which was a frequent problem in the past). |
|
I did some testing in this PR, and here’s what I’m seeing in the attached screenshot. I know it's still a work in progress, so I will test it again once it's ready. |
demiankatz
left a comment
There was a problem hiding this comment.
Thanks, @crhallberg, this is looking better! See below for a couple of comments based on an admittedly less-than-thorough review.
|
|
||
| if (triggerButton && $(triggerButton).is(".embed.btn")) { | ||
| // after setTimeout in Dialogue super class | ||
| setTimeout(() => { |
There was a problem hiding this comment.
How important is this focusing behavior? Feels fragile to rely on a setTimeout if there's any other possible way...
There was a problem hiding this comment.
I find the focus to be accessibility and intuition critical, since we want to take users directly to the embed code. I agree that a timeout isn't ideal but I'm fighting fire with slightly incremented fire.
There was a problem hiding this comment.
Makes sense. Do you think it's worth opening an issue about improving focus handling, so we don't lose track of the need to improve upon this (and maybe revisit default behavior, if any of it could be more accessible than it currently is)?
|
Thanks for the progress here, @crhallberg, this is looking good to me functionally speaking. One more question: would it make sense to put a "Share" heading at the top of the dialog box, for consistency with the Download dialog, and so the contents and button label match up? I certainly don't insist on this, but it feels like it might make things slightly nicer. |
|
Looks great @crhallberg! |
demiankatz
left a comment
There was a problem hiding this comment.
Looks good to me now, @crhallberg. Thanks again!
Future consideration: once we're in a position to write a help page about IIIF, maybe we can put a label on the draggable icon (like "Draggable IIIF Icon:") with a help button next to it leading to the explanation. I think that would make things feel even more consistent, and would provide access to the explanation without embedding a chunk of text in the box.
|
@crhallberg This looks good, Thanks for the work you've done here. Merging it now. |
Resolves #1308.
Manifest explanation?deferred.Double-check tab removaltabs returned