Skip to content

report: templates refactor#12759

Closed
paulirish wants to merge 5 commits into
masterfrom
report-templates-changeup
Closed

report: templates refactor#12759
paulirish wants to merge 5 commits into
masterfrom
report-templates-changeup

Conversation

@paulirish

@paulirish paulirish commented Jul 7, 2021

Copy link
Copy Markdown
Member

Currently a number of "component"s have their HTML source defined in report/assets/templates.html. However, for many embedders, including that file in the build process creates some challenges.. especially when we want to easily roll the latest Lighthouse somewhere.

I'm proposing a refactor for the stuff in templates.html: build out those items with standard DOM calls. We're already using this pattern pretty heavily in the report.


In this PR:

  • the styles that lives in templates.html move to the complete styles.css. (These styles were also adding extra headaches for some embedders ;)
  • created template components for 4 examples. scorescale, audit, opportunity, topbar.
  • created a lazy & cached thing to get the components. same basic effect as what cloneTemplate did.
  • i left out the unit test changes, but they're very straightforward and boring.

looking for any high-level feedback on this approach.

@google-cla google-cla Bot added the cla: yes label Jul 7, 2021
@connorjclark

connorjclark commented Jul 7, 2021

Copy link
Copy Markdown
Collaborator

At first blush (and subsequent ones...) this seems unmaintainable. Making such complex DOM with raw JS is pretty impossible to read. This will really burden future renderer changes.

Did you look into transforming (programmatically) the HTML we already have into a valid google3 template file? I don't have a full picture on how it'd work, and we'd still have to stub out cloneTemplate to defer to the templating engine, but that seems feasible.

Another approach might be to create these "components" from our authored-html. Perhaps we can generate .js files? quick example I worked up: https://gist.github.com/connorjclark/5f1df12ec7339aa6d4b55107c2d802d4

Or, is there some web component library that would work well for us?

@patrickhulce patrickhulce left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Woohoo let's do this! 🎉

high-level thoughts:

the styles that lives in templates.html move to the complete styles.css. (These styles were also adding extra headaches for some embedders ;)

great, love it 👍

created a lazy & cached thing to get the components. same basic effect as what cloneTemplate did.
i left out the unit test changes, but they're very straightforward and boring.

cool cool cool

created template components for 4 examples. scorescale, audit, opportunity, topbar.

Separating into JS sounds great, but I find this much harder to read and edit compared to the current template system or other alternatives like tagged template literals or JSX :/ (the SVG element creation is a particularly rough example of this IMO). Apologies for being out of the loop but do we have the embedder API requirements doc to be able to have a better idea if either of those might be an option?

@connorjclark

Copy link
Copy Markdown
Collaborator

@paulirish

Copy link
Copy Markdown
Member Author

now replaced with the approach in #12803

@paulirish paulirish closed this Jul 21, 2021
@paulirish paulirish deleted the report-templates-changeup branch May 18, 2022 22:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants