conversionxl / aybolit

Lightweight web components library built with LitElement.
https://conversionxl.github.io/aybolit/
MIT License
7 stars 8 forks source link

Explore more courses section #283

Closed pawelkmpt closed 1 year ago

pawelkmpt commented 1 year ago

Let me know if anything can be done better

https://app.clickup.com/t/865bfrhzh

github-actions[bot] commented 1 year ago

size-limit report 📦

Path Size
packages/cxl-ui/pkg/dist-web/cxl-ui.js 33.91 KB (+0.25% 🔺)
packages/cxl-ui/pkg/dist-web/cxl-ui-jwplayer.js 11.87 KB (0%)
packages/cxl-ui/pkg/dist-web/cxl-ui-playbooks.js 25.26 KB (+0.36% 🔺)
packages/cxl-ui/pkg/dist-web/vendor.js 125.6 KB (0%)
packages/cxl-ui/pkg/dist-web/cxl-ui-jwplayer.js, packages/cxl-ui/pkg/dist-web/cxl-ui-playbooks.js, packages/cxl-ui/pkg/dist-web/cxl-ui.js, packages/cxl-ui/pkg/dist-web/manifest.js, packages/cxl-ui/pkg/dist-web/unresolved.js, packages/cxl-ui/pkg/dist-web/vendor.js 197.8 KB (+0.09% 🔺)
anoblet commented 1 year ago

This component may not be a right fit for us. After taking a look at the documentation(https://vaadin.com/docs/latest/components/virtual-list) the component expects the data and template to be separated and written in JS.

<vaadin-virtual-list
  .items="${this.people}"
  ${virtualListRenderer(this.personCardRenderer, [])}
></vaadin-virtual-list>

Unlike most of our components, there would be no easy way to make content updates without having to go through the Aybolit PR process.

pawelkmpt commented 1 year ago

This component may not be a right fit for us. After taking a look at the documentation(https://vaadin.com/docs/latest/components/virtual-list) the component expects the data and template to be separated and written in JS.

So we can't just inject custom HTML like I did in Storybook example? It works ;) What do you suggest instead?

anoblet commented 1 year ago

The benefit of the component is that if you have a long list of items(grid with thousands of rows with action buttons), each row will not be added to the DOM until it is needed, and removed when it is not needed. This saves time on first load, and reduces overall memory usage.

Since we are already providing the content in the DOM, the virtual list isn't having any effect. You can verify this by inspecting the virtual list element and scroll up and down. Nothing is added or removed.

Keeping it simple is way more performant in this case since a few dozen related courses takes less time to render, and less memory used than registering and using the virtual list component.

My suggestion would be to apply the style rules that you've written to a ul element and skip the virtual list capabilities.

I wouldn't mind a second opinion on this, though I'm fairly certain this is the case :)

lkraav commented 1 year ago

Since we are already providing the content in the DOM, the virtual list isn't having any effect. You can verify this by inspecting the virtual list element and scroll up and down. Nothing is added or removed.

AFAIK @anoblet is exactly right with his diagnosis.

pawelkmpt commented 1 year ago

All clear. Thanks @anoblet for explaining!

I'd be great if Vaadin docs had such information in place...

pawelkmpt commented 1 year ago

@anoblet please take a look now