Skip to content
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

Bad user experience with the icons #1188

Closed
Neo-Zhixing opened this issue Jan 2, 2024 · 8 comments · Fixed by #1789
Closed

Bad user experience with the icons #1188

Neo-Zhixing opened this issue Jan 2, 2024 · 8 comments · Fixed by #1789
Labels
question Further information is requested

Comments

@Neo-Zhixing
Copy link

Neo-Zhixing commented Jan 2, 2024

Description

I have originally posted this as a comment here but I received no response from the maintainers. So I'm reposting this as an issue for visibility.

For context, there are currently two icon systems in nuxt/ui. One is based on tailwind css and the other is based on nuxt-icon. The user may switch between the two using a dynamic attribute.

This is a very bad design decision. Quoting @benjamincanac here for the original reasoning behind this design decision:

It's a tiny bit more work as you have to install the iconify packages for the collections you want to use but this makes icons instantly load as they are bundled in your css instead of fetching them from the Iconify API when rendering the page the first time.

So the problem we're trying to solve here is that "nuxt-icon causes icons to not instantly load", right?

And I would agree that it's a pretty bad problem. The iconify API is not the most reliable, and occasionally icons don't render on the Nuxt UI official website.

This is caused by a known issue in nuxt-icon, nuxt/icon#34 or nuxt/icon#99

Basically, nuxt-icon doesn't load the icon on the server side, and instead opt to call the iconify API from the client at all times.

So the reasonable solution to the problem of "nuxt-icon icons doesn't instantly load" would be "fix nuxt-icon so that icons can be injected and bundled at build time".

Instead of doing that, here we introduced another dependency so that icons can be injected by tailwind as a CSS icon.

What's the problem with that?

  • Dynamic icons, which was partially addressed in feat(Icon): switch to nuxt-icon with dynamic prop or app config feat(Icon): switch to nuxt-icon with dynamic prop or app config #862. feat(Icon): switch to nuxt-icon with dynamic prop or app config feat(Icon): switch to nuxt-icon with dynamic prop or app config #862 basically falls back to the nuxt-icon component when dynamic = true. But then with that, you once again lose the ability to allow icons to load instantly, which was why egoist/tailwindcss-icons was introduced in the first place.
  • Behavior consistency. nuxt-icon allows you to define custom Icon API sources using addIconProvider. egoist/tailwindcss-icons expect you to provide them in nuxt.config.ts. I can use bundle:name as the name property in "static" icons but not dynamic icons. That means for any custom icons I have to package them twice, once for static icons as an npm package and once for dynamic icons on my custom iconify server.
  • Implementation discrepancy. nuxt-icon embeds the SVG into the DOM. egoist/tailwindcss-icons uses CSS icons. monotone icons are image-mask. Image icons are background-image. That means if I want to style a subset of the paths in the SVG, I can do that with nuxt-icon, but not egoist/tailwindcss-icons.

What I'm trying to say here is that by introducing egoist/tailwindcss-icons you're significantly complicating an otherwise simple problem. Just go into nuxt-icon, help out @atinux, and fix nuxt-icon so that it can preload icons. Call useAsyncData to fetch the icons on the server side so they're available as soon as the page loads. That way you have one unified method of loading icons, instead of introducing a whole lot of mental overload for a problem as trivial as showing icons.

@Neo-Zhixing Neo-Zhixing added the question Further information is requested label Jan 2, 2024
Copy link
Member

atinux commented Jan 3, 2024

Damn I do not like your tone dear @Neo-Zhixing, small reminder that all the tools you are using to make your website are completely free AND open source, so a thank you to start is more than welcome.

What I'm trying to say here is that by introducing egoist/tailwindcss-icons you're significantly complicating an otherwise simple problem.

nuxt-icon was introduced afterward to solve another issue of dynamic icon but I do agree that this is still not the best UX.

Call useAsyncData to fetch the icons on the server side so they're available as soon as the page loads

Did you look at the code before pretending that it is an easy fix to do?

nuxt-icon already preload the icons during SSR: https://github.com/nuxt-modules/icon/blob/0aa6fd707a455ea8c416f73e1b4ef7295e12647e/src/runtime/Icon.vue#L93

If you take a look at the playground: https://stackblitz.com/edit/nuxt-icon-playground?file=app.vue

You will notice that no call to Iconify API is done and everything is inlined during SSR.

The main issue of nuxt-icon is during client-side navigation where it needs to fetch the API. This is why we decided to go with tailwindcss-icons for Nuxt UI to start because it solves this UX issue and fallback to nuxt-icon for dynamic one (an icon that take a bit of time to load is better than no icon at all).

I do plan to add a way to preload the icons in the future for nuxt-icon, but it is not an easy task as it needs to rely on a build plugin (Vite / Webpack), in the meantime, I still believe that we do have a good Icon system that is still a no brainer to use.

@Neo-Zhixing
Copy link
Author

Neo-Zhixing commented Jan 3, 2024

Umm I do want to point out that, the reason I'm complaining about the problems with the icon solution in nuxt/ui is that a lot of components in nuxt/ui-pro directly reference the icon component defined here, as seen in this comment. The comments above are directed towards the maintainers of nuxt/ui-pro who seem to be the same group of people as nuxt/ui maintainers.

And nuxt/ui-pro is by no means "free AND open source", because it's in fact neither.

As a paying customer, I'm not expecting 7x24 support but I do think that I should be able to complain about product usability. Customer stories like this one can help improve your product by allowing the developers to better understand user needs. They also serve as a guidance for future customers so they can form a more informed purchase decision.

nuxt-icon was introduced afterward

That contradicts what I'm hearing from @benjamincanac 's statement regarding how this design decision was made. #267 (comment)

Did you look at the code before pretending that it is an easy fix to do?
nuxt-icon already preload the icons during SSR

I recognize that the icons are preloaded during SSR. However, what I was suggesting is that "the icons should be available as soon as the page loads."

The problem we have now is that during client-side navigation the icons may not load immediately, and that produces disturbing results like this:
image

What useAsyncData does really well is blocking the page navigation before the data is available. I think that should be entirely possible, and it should be easy to do.

This is why we decided to go with tailwindcss-icons for Nuxt UI to start because it solves this UX issue

All we need is icon loads blocking page navigation. That solves 99% of the problem by avoiding sudden layout changes, and it should be both easier and less intrusive to the developer experience. While I recognize that no bug fix is easy, design decisions like "introducing another dependency that does the same thing as the current solution but differently" should not be made lightly and they should be well justified and weighted against other solutions.

Icon preloading is a nice to have because when you know all the values an icon may take on, all variants can be loaded at page load so that when the icon name attribute changes the icons will be able to load immediately. However, that does not take away from the immediate bugfix we can have which is using something like useAsyncData to block page loads before the icon data becomes available. This avoids the need to have two parallel icon systems.

Copy link
Member

@Neo-Zhixing What problem do you have with the default solution exactly? What is your use-case for using the dynamic mode?

@Neo-Zhixing
Copy link
Author

Neo-Zhixing commented Jan 3, 2024

@benjamincanac I had enabled the dynamic mode globally because

  1. I wanted to use fontawesome pro and other custom icons. I have already setup a private icon server and I want to use that. With nuxt-icon this is easy:
import { addAPIProvider } from '@iconify/vue'

addAPIProvider('dust', {
  resources: ['https://icon.my.domain'],
})

However, the CSS icon solution introduced a whole different codepath, meaning that I have to repackage all the icons and feed them to egoist/tailwindcss-icons separately.

  1. Styling. egoist/tailwindcss-icons doesn't inject the SVG into the DOM and instead renders the icon as css attributes. That makes it impossible to correctly style duotone icons, among many other issues caused by this behavior difference.

  2. Because some of my icons are dynamic. The icon name attribute may change after page load. An "empty state" for the icon is acceptable only in this case. Icon preloading is helpful to eliminate that "empty state", but it's not required for the icons to be in a non-empty state during page navigation.

Copy link
Member

atinux commented Jan 4, 2024

useAsyncData won't solve the issue of displaying the text, but it is actually <Suspense> doing such.

As we do have an await here: https://github.com/nuxt-modules/icon/blob/0aa6fd707a455ea8c416f73e1b4ef7295e12647e/src/runtime/Icon.vue#L100 it should wait

If the text is displayed, this mean that something wrong happened. Do you mind creating a small reproduction so we can help figure out the root cause?

Copy link
Member

Should we transfer this to nuxt-icon as it's not related to @nuxt/ui?

@Neo-Zhixing
Copy link
Author

Neo-Zhixing commented Jan 8, 2024

@benjamincanac
Please leave this issue as is. This issue is specifically about why it's a bad decision to introduce egoist/tailwindcss-icons. The nuxt-icon prerender issue is tangent to that. I'll open a separate issue in nuxt-icon.

Once nuxt-icon can prerender icons reliably and block page navigations when icons haven't been loaded, we should be able to get rid of egoist/tailwindcss-icons here because the reasons to justify adding egoist/tailwindcss-icons no longer holds true.


@atinux I did some digging this weekend and it's hard to reproduce this reliably. My website was deployed on Vercel, and about 50% of the deploys never have icons prerendered while the other 50% always do.

I suspect that because the loadIcon method from iconify has a 750ms timeout, if many requests to iconify were made in parallel from the same server during prerender, some of those requests may timeout leaving behind the raw text.

I've definitively seen icons flashing as text on some nuxt official websites before, so this is a widespread but hard to reproduce problem. You'll only see it flash into text once. Subsequent requests will not have the icon prerendered but you won't notice a flash as the icons were cached. And only the timed out icons will have this flash. Presumably this is what motivated the decision to switch to egoist/tailwindcss-icons.

In my case this phenomenon is particularly bad, because my custom icon server may require 1-2s to spin up.

This would also explain why client side navigation wasn't blocked on icon loading. When server side icon fetches failed, it does so silently, but you may end up with inconsistent states between client and server leading to hydration failure.

So far this is just my guess and it would be great if you can help validate this. I do not have enough context to make a judgement call on this.

There are two ways to fix this:

  1. Increase timeout to 100s
  2. Batch icon requests during prerender

I'll open a separate issue in nuxt-icon later.

Copy link
Member

@Neo-Zhixing We didn't introduce @egoist/tailwindcss-icons, this module was created with it. We introduced the dynamic prop recently which fallbacks to nuxt-icon for edge cases. We are not gonna drop the parsing through Tailwind CSS in any case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants