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.

Sign up for GitHub

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

Jump to bottom

HtmlPruner::removeInvisibleNodes could/should be better named? #717

Closed
JakeQZ opened this issue Sep 17, 2019 · 3 comments · Fixed by #718
Closed

HtmlPruner::removeInvisibleNodes could/should be better named? #717

JakeQZ opened this issue Sep 17, 2019 · 3 comments · Fixed by #718
Labels
cleanup discussion
Milestone
3.0.0

Comments

@JakeQZ
Copy link
Contributor

JakeQZ commented Sep 17, 2019

I propose removeInvisibleElements.

An element is a specific type of node. In documenation on MDN, W3C specifications and JavaScript, nodes which are elements are always referred to as elements.

The nodes removed by the method are necessarily elements. Therefore I think this method should be renamed before 3.0 is released and we are stuck with the name. @oliverklee, @zoliszabo, would you agree?
@JakeQZ JakeQZ added this to the 3.0.0 milestone Sep 17, 2019
@JakeQZ JakeQZ mentioned this issue Sep 17, 2019
Merged
@JakeQZ
Copy link
Contributor Author

JakeQZ commented Sep 17, 2019

A secondary question on the …Invisible… part of the name – it doesn't remove elements with visibility: hidden but in fact removes those with display: none, so perhaps this could also be conveyed more clearly in the name… (though I can't think of great alternatives right now)?

@JakeQZ
Copy link
Contributor Author

JakeQZ commented Sep 17, 2019

A secondary question on the …Invisible… part of the name … (though I can't think of great alternatives right now)

I'm leaning towards removeElementsWithDisplayNone – it does what it says on the tin…

@oliverklee
Copy link
Contributor

removeElementsWithDisplayNone

Sounds good. Let's use this name.

JakeQZ added a commit that referenced this issue Sep 17, 2019
@JakeQZ
Rename HtmlPruner::removeInvisibleNodes …
7f18d00 
… to `HtmlPruner::removeElementsWithDisplayNone`.

Resolves #717.
JakeQZ added a commit that referenced this issue Sep 17, 2019
@JakeQZ
Rename HtmlPruner::removeInvisibleNodes …
1ffddbd 
… to `removeElementsWithDisplayNone`.

Resolves #717.
@JakeQZ JakeQZ mentioned this issue Sep 17, 2019
Merged
JakeQZ added a commit that referenced this issue Sep 17, 2019
@JakeQZ
[CLEANUP] Rename HtmlPruner::removeInvisibleNodes …
897cef2 
… to `removeElementsWithDisplayNone`.

Resolves #717.
oliverklee pushed a commit that referenced this issue Sep 17, 2019
@JakeQZ @oliverklee
[CLEANUP] Rename HtmlPruner::removeInvisibleNodes … (#718)
fabcc48 
… to `removeElementsWithDisplayNone`.

Resolves #717.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
cleanup discussion
Projects
None yet
Milestone
3.0.0
Development

Successfully merging a pull request may close this issue.

[CLEANUP] Rename HtmlPruner::removeInvisibleNodes … MyIntervals/emogrifier
2 participants
@oliverklee @JakeQZ