-
-
Notifications
You must be signed in to change notification settings - Fork 202
Reincorporate iiif gallery component back into main application #1472
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
Reincorporate iiif gallery component back into main application #1472
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
demiankatz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @K8Sewell, this is off to a good start. See below for a couple of minor suggestions that may lead to further simplifications.
Also, I'm seeing a more significant issue: something about this PR is making the entire UV shorter. If you open up the preview for this PR and the current example at https://universalviewer.dev, you'll notice that the UV is smaller here. I only noticed this because I was doing side-by-side comparisons of the gallery, and I saw that the thumbnail labels are visible on the dev branch but "below the fold" here (at least, on my small laptop screen they are). I can share screen shots if you can't reproduce this. My guess is that something in the gallery component's CSS is bleeding to a higher level and affecting the entire viewer. Let me know if you need help tracking down the culprit!
src/content-handlers/iiif/modules/uv-shared-module/css/mixins.less
Outdated
Show resolved
Hide resolved
| "resolved": "https://registry.npmjs.org/@edsilv/http-status-codes/-/http-status-codes-1.0.3.tgz", | ||
| "integrity": "sha512-HLK2FS5sZqxPqD53D6hhZxC6C8THTVwlyZDZ7J0iWsrB8JmMA69m/CQuNKZc1kki9WSVeck2fXna26NL0SE7cg==" | ||
| }, | ||
| "node_modules/@edsilv/jquery-plugins": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I notice that removing the gallery component seems to have also removed the dependency on the jquery plugins. We should double-check that there is not code in the gallery component that depends on these, or else we may need to explicitly add this back as a direct dependency of UV.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added it as a direct dependency for now. I'm still investigating if it is essential to operations. Will hopefully have some more clear information on that soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that it is currently essential -- at very least, when I search for the first of the plugins (the checkboxButton function), I see that the gallery component uses it when multi-select is enabled. The problem is that I don't think anyone knows how to enable multi-selection functionality in order to properly test this.
Regardless, this is not the only plugin that is used, nor are the plugins used exclusively in GalleryComponent -- for example, the horizontalMargins plugin is used in quite a few places, including OpenSeadragonCenterPanel and HeaderPanel.
Looks to me like getting rid of this dependency would be quite a bit of work. For now, leaving it as an explicit dependency makes sense to me so we don't forget about it.
Thanks so much for taking a look! I'm seeing matching sizes since I've made a few edits and may have resolved the 'too short' issue but will double check this in the newly built preview as soon as it's available. One major difference I'm working on resolving is that the 'loading' icon displays differently on this branch than it does in dev and I'm still investigating the cause but all other issues are mostly wrapped. Thanks again for your time! Will update as soon as I've got parity. |
|
Thanks for the merge, @K8Sewell -- the test diffs are much more readable now! I've also discovered that the height problem still exists, but it has nothing to do with this PR. It appears that if I right click the preview link and open it in a new tab, it renders at the correct height... but if I left click it and navigate to it in the present tab, it comes out shorter. I have no idea what would be causing this, but I'm seeing the same behavior on multiple PRs, so I don't think you need to worry about chasing it down here. Let me know if you need any help with the loading icon or the jQuery plugin investigation; I might be able to do something before tomorrow's stand-up (maybe early tomorrow morning) if you need me to. |
| background: none; | ||
| position: relative; | ||
|
|
||
| &.loading { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible that we just want to remove this entirely and let the default .loading styles apply? Or have you already tried that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found the culprit. The variable assigned to loader-black-bg was different in the iiif-gallery-component repo. By bringing over the one from the repo and adding it to the variables got this loading icon working in parity with dev.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me now!
I'm glad to hear the height discrepancy is at least reproducible. Thanks for finding that! I did find that the |
|
I've been following the discussion, and I’ll wait for @demiankatz final thoughts in light of @K8Sewell last message before proceeding with the final testing. |
demiankatz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the progress, @K8Sewell, I think we're nearly done here. See below for one question.
| .iiif-gallery-component .main { | ||
| overflow: auto; | ||
| overflow-x: hidden; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this necessary? It looks to me like these styles are already defined in iiif-gallery-component.less. Is there a reason they need to be reasserted here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These styles were broken out into the separate styles.less file in the iiif-gallery-component repo so I was attempting to follow the style delineated logic of certain kinds of style properties living in the styles.less file instead of the component specific file, without realizing that these styles were duplicated. Thank you so much for catching that! I think to follow the pattern they should live here and this section should be updated with similar kinds of styles (positional, size, display, etc.) that currently live in the component specific style file. Would that be a better solution than removing them from here? Am also supportive of consolidating.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, to note, this style logic isn't applied consistently in the other component specific style files so perhaps better decision going forward is to compartmentalize. I'm open to suggestion on implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I favor consolidation -- if we have an entire less file dedicated to the gallery component, I'd rather see gallery-related styles in there than external to it; makes it easier to find all of the related code if we need to. But if you have a counter-argument, I'm happy to hear it! :-)
|
Tested everything and checked the file changes, looks good to me! Leaving it with @demiankatz now for his final thoughts. Thanks a lot @K8Sewell for the awesome work! |
demiankatz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me as well!
|
I've archived the gallery component and opened #1488 to track the unresolved accessibility issue. |
Summary
Returns
@iiif/iiif-gallery-componentback, from this commit in independent repository, into main UV application.Related Issue
#1399
Screenshot
TODO