Skip to content

Conversation

hoetmaaiers
Copy link
Collaborator

Documentation explaining how theme development should be done. Taking into account all remarks made in #88.

@hoetmaaiers
Copy link
Collaborator Author

Issue #100 🎉.

README.md Outdated
Install via conda:

```bash
conda install
Copy link
Collaborator

Choose a reason for hiding this comment

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

conda install yarn?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Indeed, well spotted.

README.md Outdated
conda install
```

Install alternative: https://classic.yarnpkg.com/en/docs/install#mac-stable.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe remove the mac specific anchor?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point, missed it over pasting.

- ./src/js/theme.js
- ./src/scss/theme.scss
- ./docs/\*\*/\*.rst
- ./docs/\*\*/\*.py
Copy link
Member

Choose a reason for hiding this comment

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

Are the py files inside pandas_sphinx_theme also whatched?
And the html layouts?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Currently not, but is is possible.

```bash
yarn build:production
```

Copy link
Member

Choose a reason for hiding this comment

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

Maybe mention that it is needed to run either build:dev or build:production before committing, as the generated files are included and tracked in the repo?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good suggestion, will add this.

Copy link
Member

Choose a reason for hiding this comment

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

I suppose that certainly the html layouts make sense to also watch?


```bash
yarn install
```
Copy link
Member

Choose a reason for hiding this comment

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

This you can run initially, but suppose you already did this, and at some point we update the dependencies (or you want to ensure you still have all up to date dependencies), what to do then? The same command?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Indeed the same command. Yarn writes away its exact installed dependencies in the yarn.lock file. Even loosely defined dependencies won't mess up new installations because the lock file is your explicit reference.

Copy link
Member

Choose a reason for hiding this comment

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

Can you mention this explicitly that if the dependencies were updated, you need to run "yarn install" again?

@hoetmaaiers hoetmaaiers force-pushed the feature/theme-dev-documentation branch from 71ff0ed to b808b86 Compare March 2, 2020 08:06
@hoetmaaiers
Copy link
Collaborator Author

I think without any other remarks, this PR can be merged?


{% extends "basic/layout.html" %} {# prev/next buttons #} {% macro
prev_next(prev, next, prev_title='', next_title='') %} {%- if prev %}
<a
Copy link
Member

Choose a reason for hiding this comment

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

Question here: did you use some kind of html auto-formatter? Which would be a good idea, but it seems it didn't handle the jinja templating very well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hi Joris,

Indeed the autoformatter I use (Prettier) is not playing friendly with Jinja. Currently this is disabled in new push on this PR.

@jorisvandenbossche
Copy link
Member

This will need a merge from master or rebase now the other PR is merged.

I might have missed the discussion elsewhere, but was there a specific reason for the theme.css/js -> index.css/js rename?
"theme.css" seems more common with other sphinx themes? (or at least, the readthedocs theme does it that way). If the idea is to split the scss files (which is a good idea), the combined built one can still be called "theme.css"? (I find that clearer that this is the css from the theme compared to "index.css", since you will also have css files loaded from bootstrap and from pygments

@choldgraf
Copy link
Collaborator

Can I give my +1 on merging this sooner than later? As someone who has recently tried to figure out the yarn/webpack build process I would benefit from documentation :-)

for index vs. theme, I think index is more common in the web-dev community, while theme is more common in the sphinx community. I'm happy w/ either

@hoetmaaiers
Copy link
Collaborator Author

The merge is done.

What @choldgraf says is true, in web-dev community the entry point is commonly named index.*. But if theme.* is most common in sphinx surroundings, I'm not arguing on this one.

@jorisvandenbossche
Copy link
Member

No experience here, but "index" might also be more common in projects that are "final" apps, not libraries. While this theme has still downstream users, so is in some way a library as well.

Anyway, my main concern is what is easiest to understand for users of the theme. I know that I regularly look at the website through firefox developer tools to eg check where a certain styling element is coming from (is it inherited from bootstrap, or from base sphinx, or is it defined somewhere in the theme css?) And then it is useful that the different css files that are included in the html have descriptive names (eg bootstrap.min.css, pygments.css). In that idea, it might actually even be better to use a longer name like "pydata-bootstrap-sphinx.css" (once we decide on a final name ... ;-))

@jorisvandenbossche
Copy link
Member

But let's the naming discussion not hold up better docs!

@jorisvandenbossche jorisvandenbossche merged commit e5e71f9 into pydata:master Mar 5, 2020
@jorisvandenbossche
Copy link
Member

Thanks a lot!

@hoetmaaiers
Copy link
Collaborator Author

Shall we create an issue to discuss the entry point naming? index.css vs theme.css

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