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

[Complex-Dom] Combine visibility: hidden and display: none for side TreeView hidden elements. #327

Conversation

lpardosixtosMs
Copy link
Contributor

@lpardosixtosMs lpardosixtosMs commented Oct 19, 2023

Following the discussion on #316 this PR adds a new class name to TreeView elements that should be hidden using display: none.

After this change the sidebar has 6004 elements and 2996 of them had the display: none value or are descendants of elements with display: none value.

Hosted version: https://lpardosixtosms.github.io/SpeedometerHiddenNone/#home

@lpardosixtosMs lpardosixtosMs changed the title Complex Dom. Combine visibility: hidden and display: none. [Complex-Dom] Combine visibility: hidden and display: none for side TreeView hidden elements. Oct 19, 2023
@lpardosixtosMs
Copy link
Contributor Author

Before and after results

image

Comparison to standalone versions

image

@lpardosixtosMs lpardosixtosMs marked this pull request as ready for review October 19, 2023 05:08
Copy link
Contributor

@julienw julienw left a comment

Choose a reason for hiding this comment

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

It's difficult to review without more comments in the code. I left a few comments to outline the parts where I'd need more explanations.
Thanks!

Copy link
Contributor

@julienw julienw left a comment

Choose a reason for hiding this comment

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

The comments help a lot, thank you

I still don't understand everything :D I left a few questions that I'd love to get answers about, but approving still.

resources/todomvc/big-dom-generator/src/tree-generator.js Outdated Show resolved Hide resolved
resources/todomvc/big-dom-generator/src/app.css Outdated Show resolved Hide resolved
Copy link
Contributor

@flashdesignory flashdesignory left a comment

Choose a reason for hiding this comment

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

thanks @julienw for all the questions, that made it easier to somewhat follow along.

@lpardosixtosMs
Copy link
Contributor Author

@julienw I addressed all your feedback, I'll go ahead and merge the PR now. Please let me know if there are any more changes you'd like to see in a follow-up.

@lpardosixtosMs lpardosixtosMs merged commit abea7b8 into WebKit:main Nov 1, 2023
4 checks passed
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