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.
| @@ -406,6 +406,7 @@ export default class IIIFContentHandler | |||
| } catch (e) { | |||
| this.hideSpinner(); | |||
| alert("Unable to load manifest"); | |||
| console.error(e); | |||
There was a problem hiding this comment.
Does this belong here? Seems like a reasonable thing to do, but seems out of scope for this PR.
|
Its much clearer where the manifest link is and how to copy it. |
Resolves #1308.