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

Add documentation for hasClass function #85465

Closed
wants to merge 1 commit into from

Conversation

GuillaumeGomez
Copy link
Member

Fixes #85450.

r? @jsha

@rust-highfive
Copy link
Collaborator

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 19, 2021
@GuillaumeGomez GuillaumeGomez added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label May 19, 2021
@jsha
Copy link
Contributor

jsha commented May 19, 2021

I'm not sure this makes sense as a warning for hasClass. null is falsy, which is exactly what we'd expect. If we wanted, we could coerce it to false, but of course that wouldn't solve the problem.

The core of the problem in #85438 wasn't that hasClass was being used incorrectly. It was that we had a series of if/else clauses, where one of the clauses was .. if (!hasClass(foo, "bar")) {. That clause led to the next clause, the applicable one, not firing. In other words, I think the problem was not with hasClass but with the call site.

function handleEscape(ev) {
var help = getHelpElement(false);
var search = searchState.outputElement();
if (!hasClass(help, "hidden")) {
displayHelp(false, ev, help);
} else if (!hasClass(search, "hidden")) {
searchState.clearInputTimeout();
ev.preventDefault();
searchState.hideResults(search);
}
searchState.defocus();
hideThemeButtonState();
}

@GuillaumeGomez
Copy link
Member Author

I tried to say that if the element doesn't exist, it will return something coercing to false too. But maybe it's too much... What do you think?

@jsha
Copy link
Contributor

jsha commented May 19, 2021

I think this comment isn't needed and winds up being just confusing to the reader. More concretely: when writing #85438, if I had read this comment, I still would have introduced the bug, because the comment matches what my expectations were: I expected hasClass to return falsy if the element doesn't exist. The thing I didn't realize was that we needed to proceed to the next else clause in case help doesn't exist.

@GuillaumeGomez
Copy link
Member Author

Makes perfectly sense. Closing then!

@GuillaumeGomez GuillaumeGomez deleted the hasClass-doc branch May 20, 2021 09:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ensure that hasClass is correctly used
3 participants