schmidtocean / sealog-client-vehicle

Sealog Client intended for use with vehicles deployed from research vessels such as ROVs, AUVs and HOVs
MIT License
0 stars 0 forks source link

MMT requests for review #7

Closed CorinneBassin-SOI closed 1 month ago

CorinneBassin-SOI commented 2 months ago

Some potential requests on-page navigation in the gallery view. Please @KaarelRaeis-SOI review and decide if these are appropriate to do When using the gallery option:

  1. Can we mirror or move the page navigation for the gallery pages to the top of the page from bottom? Currently, if you are loading a lot of images but just want to jump pages this can be difficult with the loading frames
  2. Can we add a jump-to-page option as opposed to just the back-and-forth arrows and the few page numbers that show up
KaarelRaeis-SOI commented 1 month ago
  1. Moved nav to top
  2. Debounced image loading
  3. Debounced keyboard navigation
  4. Removed keyboard navigation focus on gallery div as nav seemed to not be working most of the time, event listener is now global
  5. Added keyboard nav exceptions for text boxes so typing in the filter box doesn't change page

Rapidly moving through tens of pages created a lag of >60s as all pages were loaded sequentially, hence the debounce.

Now user can hold left/right key to rapidly move through pages - this drastically improved usability of the gallery

Deployed 1+2 to production - 920244843c Deployed 1-5 to Seacloud for testing(cloud-dev doesn't have images currently)

Will close when deployed to ship

KaarelRaeis-SOI commented 1 month ago

Final version rolled out to production with [6dbe2e3aa54118f80b66ec2a1b1f34149269054c] (https://github.com/schmidtocean/sealog-client-vehicle/commit/6dbe2e3aa54118f80b66ec2a1b1f34149269054c)

KaarelRaeis-SOI commented 1 month ago

@webbpinner this is something you might want to consider for a PR

webbpinner commented 1 month ago

The refactor to use getImageUrl in lowering_gallery_tab.js was a clear oversight on my part... good catch.

The refactor to use a global eventListener for the pagination was another good call. I'll be adding this to the lowering_replay and lowering_map pages as well.

Regarding the debounce functionality... That's a good optimization but is debounce the only function used from the lodash library? If so, I'd suggest adding a debounce function to the existing utils.js file and remove the lodash dependency.

Here's an article that I'll be using when this gets rolled into the vanilla version: https://stackoverflow.com/questions/36548451/underscore-debounce-vs-vanilla-javascript-settimeout.

KaarelRaeis-SOI commented 1 month ago

Hi Webb,

Thanks for the review, lodash was already a dep, didn’t install anything new.

Kaarel Kaspar Räis

Software Engineer Schmidt Ocean Institute

+372 56571501 www.schmidtocean.org

Follow us on social media! https://schmidtocean.org/social-media-resources/

On Thu, 19 Sep 2024 at 15:36, Webb Pinner @.***> wrote:

The refactor to use getImageUrl in lowering_gallery_tab.js was a clear oversight on my part... good catch.

The refactor to use a global eventListener for the pagination was another good call. I'll be adding this to the lowering_replay and lowering_map pages as well.

Regarding the debounce functionality... That's a good optimization but is debounce the only function used from the lodash library? If so, I'd suggest adding a debounce function to the existing utils.js file and remove the lodash dependency.

Here's an article that I'll be using when this gets rolled into the vanilla version:

https://stackoverflow.com/questions/36548451/underscore-debounce-vs-vanilla-javascript-settimeout .

— Reply to this email directly, view it on GitHub https://github.com/schmidtocean/sealog-client-vehicle/issues/7#issuecomment-2361181386, or unsubscribe https://github.com/notifications/unsubscribe-auth/BGFZFVWVQH2LIG5XKEDHWQ3ZXLOOLAVCNFSM6AAAAABNI2GC2SVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNRRGE4DCMZYGY . You are receiving this because you modified the open/close state.Message ID: @.***>

webbpinner commented 1 month ago

Ah... so just as an FYI it has been removed from the updated version of Sealog that I'm developing against.

CORRECTION... is isn't a direct dependency but is installed as a dependency of some other library. This is true even for the new version.

webbpinner commented 1 month ago

just updated vanilla versions:

Took a slightly different approach than used on the SIO version but hopefully when the 2 version are consolidated it will be relatively painless.

Please refer to the latest version sealog-client-vehicle (v2.3.4) to see the exact changes.