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 PST styling to version warning button #1700

Closed
Charlie-XIAO opened this issue Feb 2, 2024 · 15 comments · Fixed by #1721
Closed

Add PST styling to version warning button #1700

Charlie-XIAO opened this issue Feb 2, 2024 · 15 comments · Fixed by #1721
Labels
tag: component Issues or improvements associated with a given component in the theme tag: dependencies Pull requests that update a dependency file

Comments

@Charlie-XIAO
Copy link
Contributor

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
Copy link
Collaborator

trallard commented Feb 5, 2024

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 executablebooks/sphinx-design#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
Copy link
Collaborator

drammock commented Feb 5, 2024

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
Copy link
Collaborator

trallard commented Feb 5, 2024

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

@trallard trallard added tag: dependencies Pull requests that update a dependency file tag: component Issues or improvements associated with a given component in the theme labels Feb 5, 2024
@trallard trallard changed the title "switch to stable version" button in version warning banner is broken MAINT - Make sphinx-design a required dependency Feb 6, 2024
@trallard
Copy link
Collaborator

trallard commented Feb 6, 2024

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

@12rambau
Copy link
Collaborator

12rambau commented Feb 6, 2024

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
Copy link
Collaborator

drammock commented Feb 6, 2024

what other components are fully relying on sphinx-design ?

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

  • badges
  • button-links
  • cards
  • tabs
  • dropdowns

@12rambau
Copy link
Collaborator

12rambau commented Feb 6, 2024

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
Copy link
Collaborator

drammock commented Feb 6, 2024

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

  • 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

@trallard
Copy link
Collaborator

trallard commented Feb 6, 2024

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

Similar to the issue reported in #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
  • Make the version switcher an actual button (we can then leverage the bootstrap buttons and apply our 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

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
Copy link
Collaborator

drammock commented Feb 6, 2024

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
Copy link
Collaborator

trallard commented Feb 6, 2024

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
Copy link
Collaborator

drammock commented Feb 6, 2024

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
Copy link
Collaborator

12rambau commented Feb 6, 2024

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 trallard changed the title MAINT - Make sphinx-design a required dependency Add PST styling to version warning button Feb 6, 2024
@trallard
Copy link
Collaborator

trallard commented Feb 7, 2024

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

@drammock
Copy link
Collaborator

drammock commented May 3, 2024

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

@drammock drammock closed this as completed May 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tag: component Issues or improvements associated with a given component in the theme tag: dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants