executablebooks / sphinx-design

A sphinx extension for designing beautiful, screen-size responsive web components.
https://sphinx-design.readthedocs.io/en/furo-theme/
MIT License
198 stars 61 forks source link

Initial Feedback #6

Open chrisjsewell opened 3 years ago

chrisjsewell commented 3 years ago

Now that most of the documentation and tests are in-place time for some feedback

Main questions before moving to a 0.1 release is probably confirming role/directive names, and maybe css prefix (sd-)

This is intended to replace sphinx-panels; see the README for a section on differences. The main difference are no direct dependence on bootstrap and less direct use of css classes for users.

chrisjsewell commented 3 years ago

cc @choldgraf @jorisvandenbossche @Daltz333 @pradyunsg (how best to get colours working out the box for furo)

choldgraf commented 3 years ago

👍 will give it a look this week

Daltz333 commented 3 years ago

Looks good and I'm excited. Couple of thoughts.

One thing that I've had a bit more of a pain point (but I'm glad I can do it!) with sphinx-panels was when I needed to apply custom styles in some situations. IE: Having two buttons in a footer was a pain to figure out, and I used a hacky solution. See https://github.com/wpilibsuite/frc-docs/blob/main/source/index.rst

pradyunsg commented 3 years ago

how best to get colours working out the box for furo

I'm happy to add a bunch of CSS for this in Furo directly.

chrisjsewell commented 3 years ago

Documentation on applying custom CSS to panels or tabs

So yeh I planned to document some of the available CSS classes. But certainly I want to rely less heavily on these here, in preference to having specific directive options for things like margins, padding, colors, etc

Having two buttons in a footer was a pain to figure out

Yeh so now you could do this using grid + grid-item, as opposed to directly via div + classes.

I'm happy to add a bunch of CSS for this in Furo directly.

Cool cheers!

chrisjsewell commented 3 years ago

cc also @mbercx for the aiida stuff 😉

pradyunsg commented 3 years ago

Oh, one bit of feedback I have -- let's tone down the shadows? They're a little too... visible.

Here's an attempt from me, based off of https://tailwindcss.com/docs/box-shadow shadow:

box-shadow: rgba(0, 0, 0, 0) 0px 0px 0px 0px, rgba(0, 0, 0, 0) 0px 0px 0px 0px, rgba(0, 0, 0, 0.1) 0px 1px 3px 0px, rgba(0, 0, 0, 0.06) 0px 1px 2px 0px;
chrisjsewell commented 3 years ago

Oh, one bit of feedback I have -- let's tone down the shadows? They're a little too... visible.

I have changed the default card shadow (which also can be set with the shadow option) from sd-shadow-md to sd-shadow-sm, see https://sphinx-design.readthedocs.io/en/latest/css_classes.html#shadows

but obviously feel free to make a PR if you think they can be further improved

choldgraf commented 3 years ago

Had a look at a couple pages, a few Qs and feedback scattered below. Will try and play around with the codebase more this week.

In general

Really nice - I like the UX much better than sphinx-panels and agree it's better to control things at the directive args level rather than remembering custom CSS classes (I imagine this will also mesh with things like the LSP better). I think overall it's a big improvement.

Grid

  1. I like that grid has you specify number of total columns as the argument, I feel like that is more intuitive than specifying "number of columns taken up by each element" which is how bootstrap tends to do things (ie I think "I want 2 columns" is more intuitive than "each column should have a width of 6").
    1. That said, I feel like the card-level configuration semantically clashes a bit with this since it is called :columns: as well. Perhaps it could be called :column-width: or just :width: rather than :columns:? (I don't feel strongly about this though)
  2. Is it possible to specify auto in the grid arguments? E.g. {grid} auto auto auto 4?
  3. Is there any way to specify an empty column, so that a grid-item could create an empty space? Right now using an empty grid-item causes the next item to collapse.
  4. I think it'd be helpful to add a ## header for the various sections of the "grid" docs (happy to make a PR for this, just writing down here)
  5. Given that we've often spoken about the UX/UI problems with forcing a single number of columns across all screen sizes, I wonder if we should disallow specifying a single number for grid since it might encourage bad practices.
  6. It might be useful to allow for a :height: configuration value for the grid (or maybe a grid-item), so that you could fix this. Alternatively this could be a docs demo for using custom CSS classes / rules.

Badges and buttons

  1. Why is {badge} shortened to {bdg} but {button} is not shortened to {btn}?

General

  1. Could the default docs theme be anything other than Alabaster? That's not a very mobile friendly theme (you have to scroll to the bottom to hit the site navigation, I believe)
  2. Is there a way to set defaults for certain options? For example, I wonder if some people might want all dropdowns to "fade in" and manually specifying this everywhere would be cumbersome. (that said, this is probably not that big a deal)
  3. Is this compatible with sphinx-panels? I know the syntax is different, but if you had both extensions loaded, would they both still work as expected?

A totally random idea for a directive

I can't remember if I've mentioned this before but thought I'd mention it here. I find that these docs are a good demonstration of a common pattern we use in Sphinx docs, which is to do something like:

To use XYZ directive, use it like this:
:somekey: value
Some content

which renders like this:

```{XYZ} Title
:somekey: value
Some content

In these situations, it might be useful if we had something that would let us do something like:
```{XYZ} Title
:somekey: value
Some content

And it would display a tabs group with:

  1. Tab 1: The "rendered" version of the content of demo
  2. Tab 2: The "literal" content of demo

This might help us cut down on the amount of repetition and updating needed in the docs. I believe @pradyunsg does something similar in the Furo docs.

chrisjsewell commented 3 years ago

In general ... Really nice

thanks 😄

Grids

That said, I feel like the card-level configuration semantically clashes a bit with this since it is called :columns: as well.

I did have this as a TODO before, when :columns: was also an option on the grid as opposed to set with the argument. But yeh now I'm not sure; it is the number of columns the item spans, so I don't think column-width is correct, since that infers you are making a single column wider.

Is it possible to specify auto in the grid arguments? E.g. {grid} auto auto auto 4?

Is there any way to specify an empty column, so that a grid-item could create an empty space? Right now using an empty grid-item causes the next item to collapse.

I think it'd be helpful to add a ## header for the various sections of the "grid" docs (happy to make a PR for this, just writing down here)

Sure 👍

Given that we've often spoken about the UX/UI problems with forcing a single number of columns across all screen sizes, I wonder if we should disallow specifying a single number for grid since it might encourage bad practices.

Hmm, don't know if we need to be "forcing" presumed good practices

It might be useful to allow for a :height: configuration value for the grid (or maybe a grid-item), so that you could fix this. Alternatively this could be a docs demo for using custom CSS classes / rules.

What would the use case be?

Badges and buttons

Why is {badge} shortened to {bdg} but {button} is not shortened to {btn}?

because badges are inline roles, and so I felt brevity may be desirable. On top of this, because there is no easy/defined way to add options to roles, I went with the - delimited names e.g. bdg-link-primary-line (a link badge, colored primary, and outlined) which makes things even longer (hence not badge-link-primary-outline). For buttons though, they are directives so having longer names is not so much a problem, instead of button-link-primary-outline you have:

```{button-link}
:color: primary
:outline:

# General

> Could the default docs theme be anything other than Alabaster? That's not a very mobile friendly theme (you have to scroll to the bottom to hit the site navigation, I believe)

Yeh it is a pain that it is not nicer. The thinking was that I did not want the default to require any additional packages. For example, the sphinx-book-theme currently still pins to sphinx v3 (I am putting that on you to fix 😉)

> Is there a way to set defaults for certain options? For example, I wonder if some people might want all dropdowns to "fade in" and manually specifying this everywhere would be cumbersome. (that said, this is probably not _that_ big a deal)

It would certainly be possible/easy to add configuration to change the default of any directives options.
However, a reason why I am a little hesitant, is that it would mean, to understand how to render these directives you would also need to parse/read the configuration file and extract variables from there.
With an eye on our Javascript work (and with the sphinx configuration being in python) this makes it more tricky

> Is this compatible with `sphinx-panels`? I know the syntax is different, but if you had *both* extensions loaded, would they both still work as expected?

Not right now (`dropdown` being the main "clash"), there you can set the directive to override anyone previously defined. But aiding people to transition from sphinx-panels to sphinx-design probably needs more thought.
Maybe have a dummy `panels` directive in sphinx-design which just raises a warning that this is no longer supported and points to documentation on how to do this, i.e. when we replace sphinx-panels with sphinx-design in jupyter-book

> A totally random idea for a directive ...

So firstly, I disagree that the rendered output should be part of the tab set, since this means you cannot view the source and rendering at the same time. Indeed, most documentation have these separately; furo, https://getbootstrap.com/docs/5.0/layout/grid/, https://material-ui.com/components/grid/.

I have basically done something similar to https://material-ui.com/components/grid/; which has the rendered version, then a dropdown for the source where you can toggle between (in their case) Javascript and Typescript.
(To exactly copy what they have done, it may in fact be interesting to about having an "integrated" dropdown+tabs bar)

e.g. all the examples look like this:

:::{div} sd-text-center sd-font-italic sd-text-primary Some CSS styled text :::

:icon: code
:color: light

````{tab-set-code}
```{literalinclude} ./snippets/myst/div-basic.txt
:language: markdown
:language: rst


Then, the offer nifty part of this is that, in https://github.com/executablebooks/sphinx-design/blob/main/tests/test_snippets.py, I have set it up so that it runs through all the documented snippets, checks that they run for both MyST and RST, and also checks that the output AST is the same for both, i.e. the snippets are essentially the test suite and are "guaranteed" to work.
ChaiByte commented 3 years ago

Wow. It there an simple roadmap so users could know when to transfer from sphinx-pannel?

chrisjsewell commented 3 years ago

It there an simple roadmap so users could know when to transfer from sphinx-panel?

So essentially I would say it is ready to use now 😄 I don't envisage any major changes going forward, apart from things discussed in this issue (I've basically used up all the time I have to dedicate to this lol)

There are some notes on transferring here: https://github.com/executablebooks/sphinx-design#comparison-to-sphinx-panels, but obviously any feedback to this is welcome This is really the last piece of the puzzle; making it as seamless as possible to transfer, particularly for when we migrate from sphinx-panels to sphinx-design in https://github.com/executablebooks/jupyter-book

chrisjsewell commented 3 years ago

One (probably) final feature added, a scrollable card carousel: https://sphinx-design.readthedocs.io/en/sbt-theme/cards.html#card-carousels

MarDiehl commented 3 years ago

This is feedback from a sphinx-panels user, but I think it is valid for sphinx-design as well: I was surprised to see that the grid system uses different labels (“xs sm md lg”) than Bootstrap (xs sm md lg xl xxl), especially since the documentation explicitly states 'See the Bootstrap Grid system for further details.' At least for sphinx-panels, I got the intended behavior only when using the Bootstrap labels (col-xl-2 col-lg-2 col-md-3 col-sm-3 col-4).

chrisjsewell commented 3 years ago

Hi @MarDiehl note sphinx panels derived from bootstrap v4 and the documentation you refer to is for v5, where for example they first introduced xxl. In sphinx-design you no longer directly uses these classes, so I don’t think it is as much an issue, although I might eventually allow a fifth number for xxl

MarDiehl commented 3 years ago

@chrisjsewell thanks for the quick feedback (and of course for sphinx-panels and sphinx-design).

I was actually referring to bootstrap v5 because the sphinx-design documentation states See the Bootstrap Grid system for further details. Maybe I'm just biased because I reported the same issue for sphinx-panels (where the same classes as in bootstrap v4 are used if I understand it correctly), but a clear hint in the sphinx-design documentation in the spirit of See the Bootstrap Grid system for further details (except for the column labels) would help to avoid confusion.

damian-krawczyk commented 3 years ago

@chrisjsewell awesome job!

What I've found problematic for me is that the internationalization is not working for buttons - translated button changes to text (sphinx-panels has the same problem). Maybe I'm missing something to make it work for buttons?

Here I've build an example:

source page in English translated page in Polish
image image

button

source page with button in English

https://sphinx-internationalization-test.readthedocs.io/en/latest/#buttons

translation of button from English to Polish

https://github.com/damian-krawczyk/sphinx-internationalization-test/blob/ba6305e61eb3c57a050ff2064b70b82aa1fa93f0/docs/locale/pl/LC_MESSAGES/index.po#L37-L39

#: ../../index.rst:19 645df2e517a14ebe83fd8f2aa81331dd
msgid "Example button"
msgstr "Przykładowy przycisk"

translated page with button in Polish

https://sphinx-internationalization-test.readthedocs.io/pl/latest/#buttons

badge

source page with badge in English

https://sphinx-internationalization-test.readthedocs.io/en/latest/#badges

translation of badge from English to Polish

https://github.com/damian-krawczyk/sphinx-internationalization-test/blob/db2ac19c92d19a564070c03e7b9561d312d0c89d/docs/locale/pl/LC_MESSAGES/index.po#L45-L47

#: ../../index.rst:34 3651548d5be74a61ac96278b1ec9998d
msgid ":bdg-link-primary-line:`Example badge <https://example.com>`"
msgstr ":bdg-link-primary-line:`Przykładowa odznaka <https://example.com>`"

translated page with badge in Polish

https://sphinx-internationalization-test.readthedocs.io/pl/latest/#badges

damian-krawczyk commented 3 years ago

Looks like I can't set grid from 1 to 2?

.. grid:: 1 2 3 4 <- this works .. grid:: 1 2 <- this doesn't work

damian-krawczyk commented 3 years ago

Card carousels - it's hard to move it left on smartphone screen.

chrisjsewell commented 3 years ago

Thanks for the feedback @damian-krawczyk , some quick notes

Looks like I can't set grid from 1 to 2?

it feels like you are misunderstanding what these numbers represent? They are the number of columns at different screen sizes

t's hard to move it left on smartphone screen.

they are designed to “snap to card edge”, so you have to move them at least half the width of a card to snap to the next card

damian-krawczyk commented 3 years ago

just got it 👍🏻 I need .. grid:: 1 2 2 2 - and works fine, I wanted to remove my comment but you have been faster 🚀 , thanks for additinal clarification.

carousel - I'm still not convinced, try to refresh page and then move it left. It looks like that to activate the carousel I need to touch the left half of the first card and move my finger fast, then it starts working normally, as you described 🤔

stefanodavid commented 3 years ago

Hi @chrisjsewell & all

I realised just a few days ago that sphinx-panel was going to be replaced and I still had no chance to test it out. Since I rely quite heavily on panels in my sphinx projects, I second this suggestion:

Maybe have a dummy panels directive in sphinx-design which just raises a warning that this is no longer supported and points to documentation on how to do this, i.e. when we replace sphinx-panels with sphinx-design in jupyter-book

I think a warning could be useful to make sure one migrates all the panels:: directives. I also hope that the deprecation won't be any time soon, because I would probably not be able to migrate my projects before a couple of months...

Finally, let me thank you and the whole team for the great job, I had only a quick look at sphnx-design and it seems more flexible and usable thatn sphinx-panels, I hope to find the time to test it out ASAP and provide my feedback.

damian-krawczyk commented 3 years ago

Can you add modification date to article-info?