VEuPathDB / web-monorepo

A monorepo that contains all frontend code for VEuPathDB websites
Apache License 2.0
2 stars 0 forks source link

Map: there should be a spinner when changing marker overlay variable #142

Closed bobular closed 1 year ago

bobular commented 1 year ago

Don't tackle this until https://github.com/VEuPathDB/web-monorepo/pull/143 is complete! Changes to default overlay behaviour are happening.

When changing to a high-cardinality variable such as PopBio MegaStudy's Sample.species, the user has no idea that behind the scenes a request is being made to the distributions endpoint to determine the top 7 values to use as default.

We could add a spinner somewhere like in the logo and wire it up somehow to useStandaloneMapMarkers. That hook could probably also return markerVariableBundlePromise.pending which could control the legend spinner (low cost if it works)

Or the spinner could go in the marker config menu panel?

adnauseum commented 1 year ago

@bobular What do you think about something like this:

Legend with spinner

bobular commented 1 year ago

Looks like you used another spinner though.

We've been using packages/libs/components/src/components/Spinner.tsx in EDA/Map.

bobular commented 1 year ago

somewhere like in the logo

I think I meant "legend"!

adnauseum commented 1 year ago

Looks like you used another spinner though.

We've been using packages/libs/components/src/components/Spinner.tsx in EDA/Map.

Ack. We should definitely align on this. The trouble is that the spinner is out of reach:

image

We could:

Of that non-exhaustive set of options, I like not using a spinner. I used a spinner because I was being a lazy jerk. I wonder if we can achieve a slicker UI by doing something else. I like the idea of "dimming" the visibility of the options.

What do you think about this, @bobular, @asizemore, @jernestmyers, or @dmfalke ? 👇

https://github.com/VEuPathDB/web-monorepo/assets/24446573/93a3d6af-f130-4e2b-8436-2b789a056dbb

I like it with italics! 🇮🇹

https://github.com/VEuPathDB/web-monorepo/assets/24446573/86e6d23c-46ac-4cfc-bd0c-b82a7439d298

dmfalke commented 1 year ago

Looks like you used another spinner though. We've been using packages/libs/components/src/components/Spinner.tsx in EDA/Map.

Ack. We should definitely align on this. The trouble is that the spinner is out of reach:

Try @veupathdb/components/Spinner

adnauseum commented 1 year ago

Try @veupathdb/components/Spinner

No dice! I was signaled that something was going on when it wasn't importing.

Screenshot image
moontrip commented 1 year ago

Well, it could be import Spinner from '@veupathdb/components/lib/components/Spinner' ?

jernestmyers commented 1 year ago

I'll only add that:

  1. @moontrip's import should work just fine
  2. I could entertain the idea of doing a dimming effect instead of the spinner, but it comes with potential debt in determining when do we dim versus spin and then implementing said decision!
  3. Of the screencasts, I prefer the non-Italics, but I have no idea why. 😄
adnauseum commented 1 year ago

I'll only add that:

  1. @moontrip's import should work just fine
  2. I could entertain the idea of doing a dimming effect instead of the spinner, but it comes with potential debt in determining when do we dim versus spin and then implementing said decision!
  3. Of the screencasts, I prefer the non-Italics, but I have no idea why. 😄
  1. It does! Thanks @moontrip
  2. Well put. I wonder if we should be so bold as to generally prefer skeleton loading states over spinner.
  3. That's enough for me!
dmfalke commented 1 year ago

Well put. I wonder if we should be so bold as to generally prefer skeleton loading states over spinner.

I'm for this, generally speaking. I've long viewed spinners and lower maintenance, but that ultimately leads to lots of spinners all over the place. We should be more intentional about where we have loading states. Skeletons probably help with that since there is a cost involved with designing them, thus we want to minimize that effort and really think about where they should go. Also, it probably lends to a better ux.

bobular commented 1 year ago

I like the dimming, but we should probably go for the spinner for now. We should try to avoid the legend changing dimensions too often, though that may be solved in https://github.com/VEuPathDB/web-monorepo/issues/216 ?

Is there a PR for this?

bobular commented 1 year ago

Just found it - https://github.com/VEuPathDB/web-monorepo/pull/232 - I will link them