Skip to content
This repository has been archived by the owner on Apr 6, 2023. It is now read-only.

feat(nuxt): add <NuxtLoadingIndicator> component #5121

Merged
merged 13 commits into from
Jul 7, 2022
Merged

Conversation

antfu
Copy link
Member

@antfu antfu commented May 24, 2022

πŸ”— Linked issue

resolves https://github.com/nuxt/framework/discussions/2100

Based on @atinux's implementation.

❓ Type of change

  • πŸ“– Documentation (updates to the documentation or readme)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

@netlify
Copy link

netlify bot commented May 24, 2022

βœ… Deploy Preview for nuxt3-docs canceled.

Name Link
πŸ”¨ Latest commit eac0b61
πŸ” Latest deploy log https://app.netlify.com/sites/nuxt3-docs/deploys/62c71ef784c2200008fddb93

@pi0 pi0 added the pending label May 24, 2022
@pi0
Copy link
Member

pi0 commented May 24, 2022

Thanks for the PR @antfu. As we discussed in discord earlier, it might worth to pend this addition to base component based on a stable API for progress handling (Old discussions why needed: nuxt-community/axios-module#247, nuxt/http#105)

Also we would probably rename component to something more generic such as <NuxtLoadingIndicator> so that user's can override indicator behavior with something that is not essentially represented as a bar.

@pi0 pi0 changed the title feat: introduce <NuxtLoadingBar/> feat: add loading indicator component May 24, 2022
@pi0 pi0 marked this pull request as draft May 24, 2022 14:19
@atinux
Copy link
Member

atinux commented May 25, 2022

So what are the next steps?

I believe this is still a good improvements in term of UX and is completely optional.

I think we can also see progressively about the usage and feedback from users for improving it step by step without breaking it.

@antfu
Copy link
Member Author

antfu commented May 25, 2022

We could use provide / inject and a wrapper utility useLoadingIndicator() to expose the API for the module to control it. But that would rather be an advanced usage to me, and probably not blocking this have this for now.

Copy link
Member

atinux commented May 25, 2022

I agree. I prefer the useLoadingBar() naming instead of indicator, or it should be named useProgressIndicator().

Would love also your feedback on naming @danielroe

@pi0
Copy link
Member

pi0 commented May 27, 2022

In order to progress on this PR, we have to finalize naming at least.

Introducing core API for loading state can be in a follow-up PR but we should consider it changes the usage scope of the component as the current component depends on pages: hooks and works with pages/ directory only. And once we introduce core-api, it will be a universal component also with different behavior which triggers on data-fetching calls in addition to page navigations.

Regarding naming, while progress and bar represent built-in default behavior, the bar isn't essentially a representation of a customized indicator such as a rotating circle and Nuxt should be open to that or even provide customizable built-ins of different indicators! Using bar in the name prevents progress on this. API could be open to that and we had this chance only during v2 to v3 migration for improvement.

As summary:

  • This PR: Keep minimal but decide on a future-proof naming for the component to land ASAP
  • Followup PRs:
    • Introduce core API for loading indicator and refactor component to depend on it
    • Bind data-fetching composables to communicate with core API
    • Investigate different methods of overriding default indicator

Copy link
Member

atinux commented May 30, 2022

I agree in the long term vision for sure!

For naming, I would vote for <NuxtProgressIndicator /> component, see https://en.wikipedia.org/wiki/Progress_indicator

@atinux
Copy link
Member

atinux commented May 31, 2022

I believe also that we need to change the place of page:start hook to be called before the middleware execution.

Some apps may use middleware for some data fetching.

@pi0
Copy link
Member

pi0 commented May 31, 2022

I love the <NuxtProgressIndicator> naming πŸ’― Only it is not always percentage which was initially suggesting "Loading" as <NuxtLoadingIndicator> is which a broader definition of pending tasks (we currently even don't use a percentage but timers). What do you think would be finally the best?

Regarding ordering, for sure. We can either do the improvement here or later PR. I'm fine with both :) Anyway, the whole hooking system of the component needs to be changed once we migrate to the core API.

@atinux
Copy link
Member

atinux commented Jun 3, 2022

NuxtLoadingIndicator seems perfect, you are right since we cannot know about the percentage

@antfu
Copy link
Member Author

antfu commented Jun 6, 2022

To have the ability to do full customization, maybe we could use slot? Something like:

<NuxtLoadingIndicator v-slot="{ percent, show, color }">
  <div>Custom rendering</div>
</NuxtLoadingIndicator>

So users could have the freedom to now present as a bar, without the need to handle the timers themselves.

@pi0 pi0 removed the pending label Jun 8, 2022
@pi0 pi0 marked this pull request as ready for review June 8, 2022 19:59
@pi0 pi0 changed the title feat: add loading indicator component feat: add <NuxtLoadingIndicator> component Jun 13, 2022
@pi0 pi0 changed the title feat: add <NuxtLoadingIndicator> component feat(nuxt): add <NuxtLoadingIndicator> component Jun 13, 2022
@shiva-prasad-reddy
Copy link

In order to progress on this PR, we have to finalize naming at least.

Introducing core API for loading state can be in a follow-up PR but we should consider it changes the usage scope of the component as the current component depends on pages: hooks and works with pages/ directory only. And once we introduce core-api, it will be a universal component also with different behavior which triggers on data-fetching calls in addition to page navigations.

Regarding naming, while progress and bar represent built-in default behavior, the bar isn't essentially a representation of a customized indicator such as a rotating circle and Nuxt should be open to that or even provide customizable built-ins of different indicators! Using bar in the name prevents progress on this. API could be open to that and we had this chance only during v2 to v3 migration for improvement.

As summary:

  • This PR: Keep minimal but decide on a future-proof naming for the component to land ASAP

  • Followup PRs:

    • Introduce core API for loading indicator and refactor component to depend on it
    • Bind data-fetching composables to communicate with core API
    • Investigate different methods of overriding default indicator

In SPA, i have auth-middleware which does a onetime auth on a page reload, so i have added a loading indicator using page:start and page:finish hooks, i found that the page hooks are being called after the middleware is run so facing a white-screen delay.
Is there anyway that i could add some html or loading component to body in middleware and remove it at the end of middleware.

Copy link
Member

atinux commented Jun 19, 2022

I believe we should also update to make sure the page:start is called before the middleware, what do you think @danielroe ?

@danielroe
Copy link
Member

It does make sense to start this before middleware but I don't think we can move that hook. What about adding another one before middleware, and then finish loading on page: finish as before?

Copy link
Member

atinux commented Jun 21, 2022

Makes sense, can we expose middleware:start and middleware:finish hooks in this PR? If so, where shall they be added @danielroe ?

@danielroe
Copy link
Member

They will need to be added in:

  • packages/nuxt/src/app/plugins/router.ts
  • packages/nuxt/src/pages/runtime/router.ts

Copy link
Member

atinux commented Jun 21, 2022

@antfu can you add them? 😊

@antfu
Copy link
Member Author

antfu commented Jun 21, 2022

I talked with @pi0 once, we were discuss about using something like progress:start, progress:end to be more specific to the loading indicator. Should we introduce both general ones and specific ones?

One other thing is that I am a bit concerned about more runtime hooks added to the bundle will cause the payload size to increase for every app?

Copy link
Member

atinux commented Jun 22, 2022

What do you mean by the payload size to increase for every app? It will be the JS bundle growing, not the payload size (window.__NUXT__).

I rather see the <NuxtLoadingIndicator> acts based on our own hooks (middleware:start and page:finish) rather that some progress hooks at the moment.

Also, the devtools will be able to get some time mesurements between thoses hooks to help users speed up their apps.

@pi0
Copy link
Member

pi0 commented Jun 22, 2022

I think Anthony refers to the bundle size for adding more bundle code (and also there is a slight runtime overhead of calling them)

Having middleware: hooks, in general, is a nice addition (and we can introduce it in a separate PR)

As of for controlling progress it might be trickier to depend state on multiple hooks (pages:, middleware: and later on data: for showing progress for pending asyncData fetchers). What would be the start and end of progress based on? middleware or pages or wait for both to end? In general, this is why I was insisting to introduce the loading API first to avoid having such concerns directly tied to the component implementation. Progress support was always tricky in Nuxt 2 when it comes to multiple sources of progress.

Copy link
Member

atinux commented Jun 22, 2022

Well so far in both Nuxt 3 we have between two routes: Middleware -> Suspense(Pages + Components)

By using page:start and page:finish we are missing the Middleware, but this mean that is a middleware is aborting the navigation, we may not have the page:finish hook to stop the progress πŸ˜…

What you suggest is then to have the component listening to both loading:start and loading:finish and we call them directly in Nuxt code (but also giving freedom to end user to call them)?

@pi0
Copy link
Member

pi0 commented Jun 22, 2022

What you suggest is then to have the component listening to both loading:start and loading:finish and we call them directly in Nuxt code (but also giving freedom to end-user to call them)?

[for the future] Yes. We render components based on loading: hooks (also loading:progress(percent)). Users don't have to call the hooks directly but use the API such as useNuxtProgress() to announce their tasks and progress on them individually. Nuxt will have to aggregate them. Both middleware system and pages announce their task to the same API as well.

Well so far in both Nuxt 3 we have two routes: Middleware -> Suspense(Pages + Components)

Makes sense. I think for this PR to move forward, if you like to also include progress for middleware, we can start with middleware:start and end with pages:finish. I would prefer to have a separate PR to introduce middleware: hooks for changelog clarity but that shouldn't be a blocker.

Copy link
Member

atinux commented Jun 22, 2022

What we can do is to move forward by merging this PR, few people use middleware to fetch data at the moment anyway.

Then introducing the middleware PR and add support for it for our loadingIndicator component.

@pi0
Copy link
Member

pi0 commented Jun 22, 2022

Nothing is blocking if middleware is not concern :) Already plan to locally test and merge for RC.5.

@pi0 pi0 force-pushed the feat/nuxt-loading-bar branch from 5402cd4 to eac0b61 Compare July 7, 2022 17:59
@pi0 pi0 merged commit c95a48c into main Jul 7, 2022
@pi0 pi0 deleted the feat/nuxt-loading-bar branch July 7, 2022 17:59
@pi0 pi0 mentioned this pull request Jul 11, 2022
@danielroe danielroe added the 3.x label Jan 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants