pydata / pydata-sphinx-theme

A clean, three-column Sphinx theme with Bootstrap for the PyData community
https://pydata-sphinx-theme.readthedocs.io
BSD 3-Clause "New" or "Revised" License
565 stars 304 forks source link

Add PST styling to `version warning` button #1700

Closed Charlie-XIAO closed 2 months ago

Charlie-XIAO commented 5 months ago

The "Switch to stable version" button seem to use some sphinx_design classes: sd-btn sd-btn-danger sd-shadow-sm sd-text-wrap font-weight-bold ms-3 my-1 align-baseline. If I'm not mistaken, it seems that one must include sphinx_design in the extension list to get those sphinx_design style sheets.

image

Is it possible that we do not rely on the sphinx_design button, and use for instance a bootstrap button instead, or just plain text? (Since I think sphinx_design is only optional instead of mandatory).

trallard commented 5 months ago

sphinx-design is used to provide several components besides buttons ref: https://pydata-sphinx-theme.readthedocs.io/en/stable/user_guide/web-components.html

Is it possible that we do not rely on the sphinx_design button, and use for instance a bootstrap button instead, or just plain text? (Since I think sphinx_design is only optional instead of mandatory).

This is a bit tricky; on the one hand, Sphinx does not have a native button directive (see https://github.com/executablebooks/sphinx-design/pull/158). Thus, buttons in this theme (and, therefore, in sphinx-design are links styled to look like buttons. In that sense, using Bootstrap buttons (that are HTML elements) is not entirely straightforward.

On the other hand, this theme uses sphinx-design to provide many UI components, and to reduce the maintenance burden of creating a new set of components and instead provides some customisation via CSS only. So the behaviour you are experiencing (not using sphinx-design as a plugin and getting a link rendered instead) is expected (because of the point above that buttons are not buttons).

drammock commented 5 months ago

for me what is actionable about this issue is that sphinx-design should move from being an optional dependency (in the doc section) to a full dependency. Any reasons we shouldn't do that?

trallard commented 5 months ago

Nope this makes sense considering that some of our core components rely on it.

trallard commented 5 months ago

FYI I changed the title of this issue to better reflect the actionable item

12rambau commented 5 months ago

I'm not a big fan of adding compulsory dependencies (specifically big ones like sphinx-design) as it also makes us dependent on there release cycle (never forget we were unable to use Sphinx 6 and 7 for a long time because of myst).

why is it complicated to use a bootstrap button instead of sphinx-design classes? what other components are fully relying on sphinx-design ?

drammock commented 5 months ago

what other components are fully relying on sphinx-design ?

https://pydata-sphinx-theme.readthedocs.io/en/stable/user_guide/web-components.html

12rambau commented 5 months ago

We supercharged them to make them compatible but we don't call them per se in the template components ? e.g. you can write a full documentation without sphinx-copybutton or without sphinx-togglebutton

drammock commented 5 months ago

We supercharged them to make them compatible but we don't call them per se in the template components ? e.g. you can write a full documentation without sphinx-copybutton or without sphinx-togglebutton

ah, OK, I see what you mean. Off the top of my head IDK for sure, but after a quick search I think you are right that we don't actually rely on sphinx design for any components, except for the version warning button (the point of this issue). So yeah, I think another possible way forward would be

trallard commented 5 months ago

why is it complicated to use a bootstrap button instead of sphinx-design classes?

Similar to the issue reported in https://github.com/pydata/pydata-sphinx-theme/issues/1078, it derives from Sphinx and is designed to generate multiple outputs. In this case, there are no native nodes or directives for button elements in Sphinx. The components that we are calling "buttons" (in Sphinx design and PST by extension) are not buttons but links (if you inspect the code, you'll notice these are rendered as an <a> with due reason as they are not performing a programmable action, nor it has any of the required button attributes. Changing these "links-styled-as-buttons" to "buttons" will require substantial changes to a) the purpose b) the directives c) how we generate them in PST. And for most use cases, the link-styled-as-button is how people intend to use these anyway (at least within a Sphinx generated documentation site).

Here comes the catch - when @gabalafou and I were discussing link-that-are-styled-like-buttons a while back, we argued that perhaps the only component that would warrant being a button was the version switcher but decided not to go down the route of adding a new component (at least at that time and because that would more than likely break things for a good amount of users).

Now that we are discussing buttons and dependencies, I see two paths:

  1. Proposed by @drammock
  • remove sphinx-design classes from that warning button (and replace with equivalent styling)
  • document a bit more clearly on the "web components" page that if you choose to install sphinx design and use its capabilities, we've done some work to ensure compatible styling

Where the component in question will remain a link-styled-as-a-button, we must ensure we provide the styling.

  1. The longer route @gabalafou and I discussed a while back but discarded

Either route would address both @12rambau 's concerns regarding adding another dependency and the situation where people do not want to add another plugin (namely sphinx-design)

🔗 Because links and buttons are different in purpose and implementation, I suggest reading this excellent resource for a friendly deep dive

drammock commented 5 months ago

Changing these "links-styled-as-buttons" to "buttons" will require substantial changes to a) the purpose b) the directives c) how we generate them in PST.

I was interpreting @12rambau's idea as "use bootstrap classes" (instead of sphinx-design classes) and not "make it a real HTML button". I think that path is fine here: it's already a "link-styled-as-button" and (as you point out @trallard) doesn't really behave like a button. Also the change shouldn't require any user intervention.

when @gabalafou and I were discussing link-that-are-styled-like-buttons a while back, we argued that perhaps the only component that would warrant being a button was the version switcher but decided not to go down the route of adding a new component (at least at that time and because that would more than likely break things for a good amount of users).

We did eventually go down that route: the version switcher dropdown is already a <button> node in the current live stable site (I forget when that happened; check git blame if anyone's interested). The version warning button (topic of this issue / is just a link to a different docs version) is an <a> node (and I think we all agree should remain so).

trallard commented 5 months ago

Right, so we are finishing the mega interactive states PR (#1564 - I reviewed the last PR yesterday and all is left now is some ci fixes), so we could perhaps sneak in a final PR to remove the sphinx-design classes from the version warning button and instead provide the styling from PST directly and keeping this as a link-button.

Does this sound like a good course of action?

drammock commented 5 months ago

we could perhaps sneak in a final PR to remove the sphinx-design classes from the version warning button and instead provide the styling from PST directly and keeping this as a link-button.

Does this sound like a good course of action?

works for me!

12rambau commented 5 months ago

I was interpreting @12rambau's idea as "use bootstrap classes" (instead of sphinx-design classes) and not "make it a real HTML button".

That's exactly what I meant!

So providing a special styling within our css is to me the best and easiest solution.

trallard commented 5 months ago

Sorry I interpreted it as making it a full button. Will work on this with @gabalafou

drammock commented 2 months ago

I think this was addressed by #1721, so closing. Feel free to reopen if I'm wrong!