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

revert: import remark-gfm library in NcRichText as module, not async #6506

Merged
merged 1 commit into from
Feb 11, 2025

Conversation

Antreesy
Copy link
Contributor

@Antreesy Antreesy commented Feb 11, 2025

☑️ Resolves

After some testing, I'm not happy with the results. async load of ~40kB comes with side effect of resizing of components, which doesn't look good in long lists (Talk)

🖼️ Screenshots

🏚️ Before

2025-02-11_10h36_17.mp4

🏡 After

2025-02-11_10h44_40.mp4

🏁 Checklist

  • ⛑️ Tests are included or are not applicable
  • 📘 Component documentation has been extended, updated or is not applicable
  • 3️⃣ Backport to next requested with a Vue 3 upgrade

@Antreesy Antreesy added 3. to review Waiting for reviews regression Regression of a previous working feature feature: richtext Related to the richtext component labels Feb 11, 2025
@Antreesy Antreesy added this to the 8.23.0 milestone Feb 11, 2025
@Antreesy Antreesy requested review from susnux and ShGKme February 11, 2025 09:49
@Antreesy Antreesy self-assigned this Feb 11, 2025
This reverts commit 49ce673.

Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>
@Antreesy Antreesy force-pushed the fix/6259/revert-async-gfm branch from bf3ade2 to 2f3ece2 Compare February 11, 2025 09:53
@susnux
Copy link
Contributor

susnux commented Feb 11, 2025

Would it not be possible to just keep showing the skeletons until loading is finished?
That would fix the visual issue and also keep bundle size smaller (also performance of parsing).

@ShGKme
Copy link
Contributor

ShGKme commented Feb 11, 2025

Would it not be possible to just keep showing the skeletons until loading is finished?
That would fix the visual issue and also keep the bundle size smaller (also performance of parsing).

We could show the text until the (I missed that it shows only a spinner). But even with the text, we cannot predict the result size, having all the headings, lists, code blocks and so on.

Copy link
Contributor

@ShGKme ShGKme left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, I wasn't fun of it initially, as mentioned in the original PR

@Antreesy
Copy link
Contributor Author

Would it not be possible to just keep showing the skeletons until loading is finished?

If you're asking whether it's possible to do on app side, we then need to trigger async import while it's loading (e.g. place NcRichText with markdown somewhere hidden to trigger it on initial load), and show skeleton until internal component state is updated? Not sure that's a good idea..

@ShGKme
Copy link
Contributor

ShGKme commented Feb 11, 2025

also keep bundle size smaller

We can separate it into two components:

  1. Simple and light text renderer with mentions and links support
  2. Heavy markdown renderer

@susnux
Copy link
Contributor

susnux commented Feb 11, 2025

It was more like:
emit some loaded event when rendering is finished so the app can replace their loading (talk skeletons) with the rendered component.

We can separate it into two components:

Do we use the non-markdown versions somewhere? Because if not then it makes not much sense.
For me the problem is the amount and size of dependencies that are synchronously imported by this component.
So for personal projects I do not use it but custom components with markdown rendering saving quite some size.

Copy link
Contributor

@susnux susnux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

anyways makes sense here

@ShGKme ShGKme added the bug Something isn't working label Feb 11, 2025
@Antreesy
Copy link
Contributor Author

Do we use the non-markdown versions somewhere?

Found some usages in Talk, Activity, integration apps, could be somewhere else. So some may benefit from it, especially if discussed strip-markdown option will be added

@Antreesy
Copy link
Contributor Author

/backport to next

@Antreesy Antreesy merged commit 8634697 into master Feb 11, 2025
23 checks passed
@Antreesy Antreesy deleted the fix/6259/revert-async-gfm branch February 11, 2025 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews bug Something isn't working feature: richtext Related to the richtext component regression Regression of a previous working feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants