Add supporting code for Kolibri loaders #448

MisRob · 2023-09-06T12:39:19Z

Description

Extracts the logic for displaying loader for a minimum amount of time from KCircularLoader to a new useKShow composable to allow for usage in v-if/v-else blocks and with any component
Relatedly renames KCircularLoader's show prop to shouldShow to prevent from naming conflicts
Adds a documentation sidebar section for all composables and adds the documentation for useKShow there
Adds pieces of guidance to docs on when to use useKShow vs KCircularLoader's minVisibleTime
Adds an option to disable the default KCircularLoader's transition to prevent from glitches when using the loader in tandem with another component, both of them wrapped in a transition
Adds a new component KTransition as a beginning of journey towards having a limited set of consistently used transitions

Issue addressed

This is all used in Kolibri's learningequality/kolibri#11201

Steps to test

Testing checklist

Contributor has fully tested the PR manually
If there are any front-end changes, before/after screenshots are included
Critical and brittle code paths are covered by unit tests
The change has been added to the changelog

Reviewer guidance

Is the code clean and well-commented?
Are there tests for this change?
Are all UI components LTR and RTL compliant (if applicable)?

- Extracts the logic for displaying loader for a minumum amount of time from KCircularLoader to a new useKShow composable to allow for usage in v-if/v-else blocks and with any component - Relatedly renames KCircularLoader's `show` prop to `shouldShow` to prevent from naming conflicts - Adds a documentation sidebar section for all composables and adds the documentation for useKShow there - Adds pieces of guidance to docs on when to use useKShow vs KCircularLoaders' minVisibleTime

marcellamaki · 2023-09-08T19:29:41Z

@MisRob - can you think of an example that is not the circular loader where the new KShow composable would be helpful? I can understand the utility in a theoretical way but I'm trying to think of an example to make it a bit more concrete for myself.

MisRob · 2023-09-09T11:33:21Z

@marcellamaki One another example in Kolibri would be global linear loaders that have the same problem as circular ones.

Even though I kept that in mind as well, the main motivation at this point was rather getting this code out of KCircularLoader in a way that will allow usage within v-if/v-else, and hopefully will be simple and open enough to keep building on top of that.

MisRob · 2023-09-09T11:46:21Z

I have an assumption that showing any kind of loader very briefly, not just a circular one, is unhappy, but there are also different techniques to deal with loading the whole page than relying on liner loaders. So I believe we'd need more input from designers.

I see two perspectives to consider here:

I'd be open to having this logic limited to circular loader if you have any ideas. My first attempt was in that direction, but couldn't figure out reasonable API that would be simple to use (and document), particularly in regard to conditional blocks. Even though there is potential to use it with linear loaders, it's true that it's not current requirement.
On the other hand, one of the main purposes of Composition API is to allow extracting pieces of logic outside of components when helpful, even when it's not necessarily re-used at many other places. So my thoughts are - why not to benefit from that.

MisRob · 2023-09-10T13:44:22Z

docs/pages/usekshow.vue

+      </ul>
+    </DocsPageSection>
+
+    <DocsPageSection title="Parameters" anchor="#parameters">


"Parameters" and "Returns" would ideally get automatically generated from JSDoc comments of the composable function, similarly how "Props" are generated from components. I believe this would be helpful for the other two composables we have in KDS already. Will open an issue for this as soon as review of this PR is done.

MisRob · 2023-09-10T13:53:38Z

docs/tableOfContents.js

@@ -170,6 +171,27 @@ export default [
      }),
    ],
  }),
+  new Section({
+    title: 'Composables',


I imagine that we'd ideally move already existing composables (responsiveWindow and responsiveElement) to this section, rather than having them hidden in the components docs, and also move their code files to the same folder. In that way, we would have consistent imports in products.

I hope having this section above components will help with discoverability of KDS Composition API utilities (see learningequality/kolibri#11132 (comment)) cc @akolson

Can open follow-up.

MisRob · 2023-09-10T14:54:57Z

Added some follow-up issues suggestions, Should be ready for review now.

MisRob · 2023-09-12T19:43:14Z

@marcellamaki By chance, I came across a different use-case today, that's even completely different from v-if/v-else blocks and shows that we might need to have this logic independent of the circular loader sooner than I thought. This will soon be CoreTable's implementation of showing a loader (learningequality/kolibri#10993 (comment)):

/*
 * If data is still loading, then we show a loader. Otherwise, we show the
 * empty message if there are no rows in the table. If we have loaded data, have
 * an emptyMessage and have no rows. If we have rows, then we show the table alone
 */
const dataStatusEl = this.dataLoading
  ? createElement('p', [createElement(KCircularLoader)])
  : !tableHasRows && createElement('p', this.emptyMessage); // Only show message if no rows

return createElement('div', { class: 'core-table-container' }, [
  createElement('table', { class: 'core-table' }, [
    ...(this.$slots.default || []),
    theadEl,
    this.dataLoading ? null : tbodyCopy,
  ]),
  dataStatusEl,
]);

Notice this.dataLoading ? createElement('p', [createElement(KCircularLoader)]) and this.dataLoading ? null : tbodyCopy placed on different lines.

If we will want to prevent jarring UX in the CoreTable by using minimum visible time logic for the loader (I would say we will want because this problem is already present in Kolibri), then it won't be enough to only use minVisibleTime on the KCircularLoader since both these conditions will need to be synchronized to prevent from glitches caused by timing inconsistency.

I think here we could apply two shows with the same ID. I'm pretty curious if it would work because I didn't think of this when working on it so perhaps will try it out if I can find a while.

Perhaps there would be a different way how to refactor CoreTable but in any case, I thought this could be a good example of a non-standard situation.

nucleogenesis

I like this because it feels like a simple wrapper for the setTimeout function built-to-purpose to be used in Vue for loading states (assuming I'm understanding everything as well as I think I am :-D)

I want to be sure I'm reading things right, so his the the gist of it, that beneath the registering w/ the keys and namespacing calls to show, it really comes down to:

Return true if shouldShow is true (no matter what)
Otherwise, return true until it's been longer than minVisibleTime
Then return false

Am I reading this right?

I'm having a hard time considering any use case for this outside of loaders because it kind of relies on a pattern of whoever is using this to be tracking a Boolean they pass in. I initially thought maybe we could use something like this for temporarily hiding/showing snackbars / errors but because those are more triggered than they are dependent on an ongoing process they work find as-is.

That said, are there any non-loader cases where we plan to use this? If this is pretty much specific to loading states it might be worth renaming it to live as part of the lib/loaders namespace? Just a thought though.

nucleogenesis · 2023-09-13T21:45:24Z

lib/loaders/KCircularLoader.vue

@@ -95,7 +102,7 @@
      /**
       * Whether the loader should be displayed or not. It needs to be used instead of `v-if/v-show` for `minVisibleTime` to work.
       */
-      show: {
+      shouldShow: {


Just noting that there is only once use of the show prop in Kolibri in the LearningActivityBar so when this merges we should target an issue in Kolibri for the next release that will use this change.

Thank you. That's updated here learningequality/kolibri@44b92da#diff-acab644da93f15633de6bae4d5b1b3fe36ef427a8641ea305eb12e9aeb50b759R43. I agree it'd be good to regression test. Alternatively, we can revert this change if we renamed the new show function to something else as mentioned in #448 (comment) and avoid this major API change.

nucleogenesis · 2023-09-13T21:54:15Z

docs/pages/usekshow.vue

+      <!-- eslint-enable -->
+    </DocsPageSection>
+
+    <DocsPageSection title="Example" anchor="#example">


This example is excellent <3

Thank you, playing around this was fun, glad that it feels useful

MisRob · 2023-09-14T06:19:29Z

@nucleogenesis

Return true if shouldShow is true (no matter)

Otherwise, return true until it's been longer than minVisibleTime

Then return false

Am I reading this right?

Almost, except of the first point. If minVisibleTime is still being applied, it will return true even though shouldShow changed to false meanwhile. That's the main point of this logic. This can be seen in the default docs example, where isFetching switches to false but loader is still visible due to the minimum visible time still running.

If you have any idea how to make this clearer, I am glad to update documentation.

MisRob · 2023-09-14T06:35:24Z

@nucleogenesis

That said, are there any non-loader cases where we plan to use this? If this is pretty much specific to loading states it might be worth renaming it to live as part of the lib/loaders namespace? Just a thought though.

I'm not aware of anything at this point. I don't see problem in applying it eventually to snackbars, but I'm not sure if I could understand exactly what you were saying in that regard so we'd need to chat more. So for now, the answer is no.

The reason for putting this into composables directory was that we have two more composables (useKResponsiveWindow, useKResponsiveDimension) in KDS and I thought it might be good to have them all in one directory so then in products, we know we can import all these utilities like

import useKSomething from 'kolibri-design-system/lib/composables/useKSomething';

I an imagine that in this way, we wouldn't need to document usage for each composable separately with each having different location like we currently do:

But perhaps it'd better to document usage in that way anyways.

An alternative approaches taking both these perspectives into account might be

Place useKShow to composables/loading/useKShow (it will be the only one though so I don't know if that's too complicated)
Keep it in lib/composables but rename useKShow to something more relevant in regards to its current usage. I believe that would also allow us to not be reanming KCircularLoaders's show prop. I tried to think about this already and would need some help with ideas :) I think this could also perhaps address @marcellamaki's point?

In any case, if you can see more benefit of keeping composables directly in lib rather than lib/composables and namespacing them there, we could do that as well. Just thinking about organization of composables in general.

nucleogenesis

This all looks great to me! One non-blocking thought -- perhaps it would be worth writing some unit tests for the useKShow composable before release to outline some of the more complicated scenarios. Although... dealing with time can be difficult so maybe that'd be more work than it's worth?

Also - re: the location of the composable, I've thought a bit about it and I think that it probably makes more sense to have the composables directory where we keep those kinds of utilities.

I think that when going to remember "where is the composable I need" it'll be so much nicer that I know it's in lib/composables than if I had to consider "what context of KDS' directory structure would this live in?". Definitely open to follow-up discussion at a tactical or something but I think this is good to go.

Thank you for all of this I really look forward to using it in Kolibri

MisRob · 2023-09-22T16:55:32Z

One non-blocking thought -- perhaps it would be worth writing some unit tests for the useKShow composable before release to outline some of the more complicated scenarios. Although... dealing with time can be difficult so maybe that'd be more work than it's worth

@nucleogenesis Heh, yes, I admit I was avoiding it in this case because of timing. There are ways to work with that in Jest though and it might not be that complicated. I will play around with at least some basic scenarios in a follow-up and we can see how it goes. Since this is something that is responsible for showing/hiding important data in tables, I agree it'd be good to have it tested. Thank you.

MisRob · 2023-10-16T15:55:27Z

@nucleogenesis Here are the tests #472. Also found quite an important bug thanks to it. Working with timing wasn't that bad. Waiting for your review.

MisRob added 4 commits September 3, 2023 10:06

Add an option to disable default transition

8097a8d

Add KTransition

339f923

Update changelog

cfd99de

MisRob mentioned this pull request Sep 7, 2023

Improve loading UX on some places in Coach learningequality/kolibri#11201

Merged

9 tasks

MisRob requested review from nucleogenesis, marcellamaki and thanksameeelian September 7, 2023 13:30

MisRob marked this pull request as ready for review September 9, 2023 11:22

MisRob commented Sep 10, 2023

View reviewed changes

MisRob added the TODO: needs review Waiting for review label Sep 10, 2023

nucleogenesis reviewed Sep 13, 2023

View reviewed changes

nucleogenesis approved these changes Sep 22, 2023

View reviewed changes

MisRob merged commit 3ab9801 into learningequality:develop Sep 25, 2023

MisRob deleted the k-show branch October 14, 2023 11:43

MisRob mentioned this pull request Oct 16, 2023

Add tests for useKShow and fix a bug #472

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add supporting code for Kolibri loaders #448

Add supporting code for Kolibri loaders #448

MisRob commented Sep 6, 2023 •

edited

Loading

marcellamaki commented Sep 8, 2023

MisRob commented Sep 9, 2023

MisRob commented Sep 9, 2023 •

edited

Loading

MisRob Sep 10, 2023

MisRob Sep 10, 2023

MisRob commented Sep 10, 2023

MisRob commented Sep 12, 2023 •

edited

Loading

nucleogenesis left a comment

nucleogenesis Sep 13, 2023

MisRob Sep 14, 2023 •

edited

Loading

nucleogenesis Sep 13, 2023

MisRob Sep 14, 2023

MisRob commented Sep 14, 2023

MisRob commented Sep 14, 2023 •

edited

Loading

nucleogenesis left a comment •

edited

Loading

MisRob commented Sep 22, 2023 •

edited

Loading

MisRob commented Oct 16, 2023

Add supporting code for Kolibri loaders #448

Add supporting code for Kolibri loaders #448

Conversation

MisRob commented Sep 6, 2023 • edited Loading

Description

Issue addressed

Steps to test

Testing checklist

Reviewer guidance

marcellamaki commented Sep 8, 2023

MisRob commented Sep 9, 2023

MisRob commented Sep 9, 2023 • edited Loading

MisRob Sep 10, 2023

Choose a reason for hiding this comment

MisRob Sep 10, 2023

Choose a reason for hiding this comment

MisRob commented Sep 10, 2023

MisRob commented Sep 12, 2023 • edited Loading

nucleogenesis left a comment

Choose a reason for hiding this comment

nucleogenesis Sep 13, 2023

Choose a reason for hiding this comment

MisRob Sep 14, 2023 • edited Loading

Choose a reason for hiding this comment

nucleogenesis Sep 13, 2023

Choose a reason for hiding this comment

MisRob Sep 14, 2023

Choose a reason for hiding this comment

MisRob commented Sep 14, 2023

MisRob commented Sep 14, 2023 • edited Loading

nucleogenesis left a comment • edited Loading

Choose a reason for hiding this comment

MisRob commented Sep 22, 2023 • edited Loading

MisRob commented Oct 16, 2023

MisRob commented Sep 6, 2023 •

edited

Loading

MisRob commented Sep 9, 2023 •

edited

Loading

MisRob commented Sep 12, 2023 •

edited

Loading

MisRob Sep 14, 2023 •

edited

Loading

MisRob commented Sep 14, 2023 •

edited

Loading

nucleogenesis left a comment •

edited

Loading

MisRob commented Sep 22, 2023 •

edited

Loading