Skip to content

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
import {renderFlowReport} from '../../../flow-report/api';

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

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
Contributor

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