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

Maint: do not use document.write #1921

Merged
merged 5 commits into from
Aug 6, 2024
Merged

Conversation

Carreau
Copy link
Collaborator

@Carreau Carreau commented Jul 9, 2024

See #1920, this removes every usage of document.write in favor of display:none with a noscript tag.

I did have to be a little more specific for buttons as the css rule in boostrap were overwriting the *.jsonly.

Note that this does not solves #1920 as some things (like the more dropdown in nav bar) still require JS to work.

@Carreau
Copy link
Collaborator Author

Carreau commented Jul 10, 2024

I'm not sure how to interpret the failures. Guidance appreciated

@drammock
Copy link
Collaborator

looks to me like:

Unexpected warning: WARNING: dot command 'dot' cannot be run (needed for graphviz output), check the graphviz_dot setting

is probably the culprit behind the a11y test failure.

@Carreau
Copy link
Collaborator Author

Carreau commented Jul 11, 2024

Oh, it's also failing on main also. I thought it was failing because of my changes.

Carreau added 2 commits July 18, 2024 11:00
See pydata#1920, this removes every usage of document.write in favor of
display:none with a noscript tag.

I did have to be  a little more specific for buttons as the css rule in
boostrap were overwriting the *.jsonly.

Note that this does not solves pydata#1920 as some things (like the more
dropdown in nav bar) still require JS to work.
Copy link

github-actions bot commented Jul 18, 2024

Coverage report

This PR does not seem to contain any modification to coverable code.

Copy link
Collaborator

@gabalafou gabalafou left a comment

Choose a reason for hiding this comment

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

This is great! Just have a few comments

-->
<noscript>
<style>
*.jsonly, *.btn.jsonly { display: none; }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the leading asterisk there for precedence? Maybe we should do:

.jsonly {
  display: none !important;
}

Also, should we prefix it with pst- and put a dash between js and only?

.pst-js-only

I'm thinking by analogy to the utility class sr-only.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

+1 to pst-, but I woulld prefer not to use !important, I have been told it is bad practice.

Copy link
Collaborator

Choose a reason for hiding this comment

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

So that goes back to my original question: is the leading asterisk for precedence?

My concern is that it might be too easy right now for some other CSS with display: flex or some other display rule to override the pst-js-only class.

I'll need to take a closer look at selector precedence and come back to this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So yes, it's for precedence.

But I think we use pst-js-only in so few places – and I believe it should be used sparingly – that it should be ok to review each of them independently.

Alternative is to not have a class but a lit of ids of elements to hide when no js.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay, so I took a little look into this.

My concern here is that in most of our day-to-day development, we're not going to see this class in action. So it could be fairly easy to break it without realizing it.

For example let's take the search-button component, which uses the pst-navbar-icon class. Someone might be working to fix a style issue and they realize that a Bootstrap rule is being applied that they need to override, so they increase the specificity of the set of pst-navbar-icon rules by giving the search-button an id, and targeting via that id, which then overrides your display: none rule. The developer never realizes it, though. The example is a little bit contrived, but not that much.

As for the alternative idea of targeting ids, I don't like it because I think the pst-js-only class really helps with discoverability and readability.

I really think that in this case, by the same logic you invoked earlier about pst-js-only class used in so few places and for such a specific purpose, that it's okay to use !important. The only other preventative measure I can think of would be to write tests that load the page without JavaScript.

Copy link
Collaborator

Choose a reason for hiding this comment

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

One thing to note about !important is that it reverses the order of specificity: important user-agent rules come before important user rules come before important site author rules, which means that if a user or browser wants to override our pst-js-only rule, they can.

Copy link
Collaborator Author

@Carreau Carreau Jul 30, 2024

Choose a reason for hiding this comment

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

removed the *, and added !important. the .btn was still needed though.

Copy link
Collaborator

@gabalafou gabalafou left a comment

Choose a reason for hiding this comment

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

Typo in the CSS rule. That's why you needed the .btn prefix.

Co-authored-by: gabalafou <gabriel@fouasnon.com>
@gabalafou gabalafou merged commit 39aab2a into pydata:main Aug 6, 2024
29 checks passed
@Carreau
Copy link
Collaborator Author

Carreau commented Aug 6, 2024

Thanks !

Carreau pushed a commit that referenced this pull request Oct 1, 2024
As reported in
#1997 - this PR
removes some cruft leftover from #1921

This should be merged ideally before cutting the `0.16.0` release.
gabalafou pushed a commit to gabalafou/pydata-sphinx-theme that referenced this pull request Oct 6, 2024
As reported in
pydata#1997 - this PR
removes some cruft leftover from pydata#1921

This should be merged ideally before cutting the `0.16.0` release.
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.

3 participants