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

misc: deduplicate all the dom helpers #15960

Merged
merged 2 commits into from
Apr 17, 2024
Merged

misc: deduplicate all the dom helpers #15960

merged 2 commits into from
Apr 17, 2024

Conversation

connorjclark
Copy link
Collaborator

fixes #11992

This reduces 4 instances of find to 1. I also adopted treemap's default second param to the canonical instance in DOM.

@connorjclark connorjclark requested a review from a team as a code owner April 17, 2024 19:34
@connorjclark connorjclark requested review from adamraine and removed request for a team April 17, 2024 19:34
@@ -15,25 +15,13 @@ import {ReportRenderer} from '../../../report/renderer/report-renderer.js';
import {TextEncoding} from '../../../report/renderer/text-encoding.js';
import {renderFlowReport} from '../../../flow-report/api';

// @ts-expect-error Legacy use of report renderer
const dom = new DOM(document);
Copy link
Member

Choose a reason for hiding this comment

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

Why not just add document.documentElement to get rid of the type error?

Copy link
Collaborator Author

@connorjclark connorjclark Apr 17, 2024

Choose a reason for hiding this comment

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

because its more work than just that

image

so i'll do it in a separate PR

#13821

@brendankenny
Copy link
Member

Could it be a standalone function that these files could import and that dom.js could use as well? Feels weird to pull in and instantiate all of DOM for this one function that's not static only for (extremely) historical reasons.

@connorjclark
Copy link
Collaborator Author

That's a reasonable further refactor. Note that treemap uses createElement/createChildOf too.

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

Successfully merging this pull request may close these issues.

"find element" code is duplicated in many places
4 participants