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

[Big-dom-generator] Change SVG to <use> element and Fix dom node count #295

Conversation

issackjohn
Copy link
Contributor

@issackjohn issackjohn commented Aug 16, 2023

Changes in this PR include:

  • Extracting common files from the build scripts in the complex examples to the shared BuildComplex utility function.
    • We have not extracted out "node_modules/big-dom-generator/dist/big-dom-generator.css" because although it is shared by the build scripts. lit-complex & javascript-web-components-complex will not use it and it will have to be removed put back in the indivisual build scripts.
  • Fixed a bug where the number of nodes in the tree-area were not being correctly counted resulting in a lot more nodes than the TARGET_SIZE specified in resources/todomvc/big-dom-generator/params.js
  • TaskListIcon was converted to an SVG with a use element inside of it because each TaskListIcon would have 6 children. This would result in a significant amount of the children in the tree-area coming from the SVG which is not desired. The ChevronRight is not a problem because it only has 1 child.
  • Rebuild the complex dists

NOT READY FOR REVIEW [WIP]

issackjohn and others added 2 commits August 16, 2023 14:46
* Replaced generated css with complex handcrafted CSS 📖  (#44)

---------

Co-authored-by: Luis Fernando Pardo Sixtos <[email protected]>
Co-authored-by: lpardosixtosMs <[email protected]>
* use syntax for one svg, tasklist
* update build scripts
* fix dom size counter (#54)
* remove not needed attributes
* update to node counting
* build all complex dists

---------

Co-authored-by: Luis Fernando Pardo Sixtos <[email protected]>
Co-authored-by: lpardosixtosMs <[email protected]>
* hide the horizontal overflow
* regen dists
@julienw
Copy link
Contributor

julienw commented Aug 18, 2023

TaskListIcon was converted to an SVG with a use element inside of it because each TaskListIcon would have 6 children. This would result in a significant amount of the children in the tree-area coming from the SVG which is not desired. The ChevronRight is not a problem because it only has 1 child.

Hey, I've seen this and was wondering why you decided to implement this change. Is there some real-world data backing up this change?

I did see some versions of this, but that can be subtly different. For example some are using <use> with an external file (eg decathlon.fr, fnac.com), while others are using it with using a hash to refer to an id for a <symbols> (eg: ebay.com). My understanding is that the performance characteristics can be very different between <use> and plain SVG, and between these 2 cases, so it would be good to be more explicit.
I didn't do a thorough search on websites so I was wondering if you did.

Thanks for your answer.

@lpardosixtosMs
Copy link
Contributor

TaskListIcon was converted to an SVG with a use element inside of it because each TaskListIcon would have 6 children. This would result in a significant amount of the children in the tree-area coming from the SVG which is not desired. The ChevronRight is not a problem because it only has 1 child.

Hey, I've seen this and was wondering why you decided to implement this change. Is there some real-world data backing up this change?

I did see some versions of this, but that can be subtly different. For example some are using <use> with an external file (eg decathlon.fr, fnac.com), while others are using it with using a hash to refer to an id for a <symbols> (eg: ebay.com). My understanding is that the performance characteristics can be very different between <use> and plain SVG, and between these 2 cases, so it would be good to be more explicit. I didn't do a thorough search on websites so I was wondering if you did.

Thanks for your answer.

We realized that around a third of the elements of the generated DOM were coming from these SVG items, this was not intended and I felt we should need a good reason to have that many svg elements. We didn't do a thorough search to justify going with use instead, beyond the fact that it generates less elements and that it is used in some sites. I don't have a preference for either solution, and earlier perf runs suggested that there was not much of a perf difference (at least in the measured time). But my latest run was different so I'm keeping this as draft until I can dig a little bit into the performance.

Perhaps the we could avoid the use syntax and also the large amount of svg children by just using a simpler icon instead.

@rniwa
Copy link
Member

rniwa commented Aug 22, 2023

Do we have any data which suggests that SVG use element is commonly used?

@lpardosixtosMs
Copy link
Contributor

Do we have any data which suggests that SVG use element is commonly used?

@rniwa Thanks for asking. I did a quick search across 20 sites, and I only found 3 using the <use> tag. I'm ok with dropping the <use> syntax given the low usage.

Sites visited:

  • amazon.com
  • youtube.com
  • Google results page
  • Stack overflow post
  • reddit.com
  • bestbuy.com
  • MDN (promises article)
  • Github (speedometer repo).
  • Bing results page
  • walmart.com
  • apple.com
  • facebook.com
  • x.com
  • gmail.com
  • play.max.com
  • ebay.com
  • airbnb.com
  • cbc.ca/news
  • bbc.com/news
  • nationalgeographic.com

Sites using the <use> tag:

  • youtube.com
  • ebay.com
  • naitonalgeographic.com

@lpardosixtosMs
Copy link
Contributor

Closing this PR. I'll open a new one for the bug fix only.

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

Successfully merging this pull request may close these issues.

4 participants