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

Do not generate dropdown in sidebar. #1771

Merged
merged 3 commits into from
May 8, 2024
Merged

Conversation

Carreau
Copy link
Collaborator

@Carreau Carreau commented Apr 15, 2024

This should fix #1735, it is complementary to #1564 but with a different approach.

Instead of generating the same navbar as the fullpage width one, it generate one with no dropdowns, with this, any css that tries to make downtown looks like normal links in html becomes unnecessary.

@trallard trallard requested a review from gabalafou April 16, 2024 16:58
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.

I left a few suggestions and comments

src/pydata_sphinx_theme/toctree.py Outdated Show resolved Hide resolved
as well as the list of links to but in a dropdown.

Returns:
- HTML str for the navbar
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it would be a simple refactor that might help make this function easier to understand if it returns

  • list of HTML str for navbar
  • list of HTML str for the dropdown

Copy link
Collaborator Author

@Carreau Carreau Apr 18, 2024

Choose a reason for hiding this comment

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

I think better than that, the cached function should not return html but the data necessary to build the html. But that require some refactor.

I'll make an issue to suggest doing a refactor for later.

Edit: for completeness this was done as issue #1773

src/pydata_sphinx_theme/toctree.py Show resolved Hide resolved
@@ -32,7 +32,9 @@ def config_provided_by_user(app: Sphinx, key: str) -> bool:
return any(key in ii for ii in [app.config.overrides, app.config._raw_config])


def traverse_or_findall(node: Node, condition: str, **kwargs) -> Iterator[Node]:
def traverse_or_findall(
node: Node, condition: Union[Callable, type], **kwargs
Copy link
Collaborator

Choose a reason for hiding this comment

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

Oops this doesn't seem related to the other changes. Could you move this to a different PR please?

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.

Looks like some unrelated changes got pushed

@trallard trallard added tag: CSS CSS and SCSS related issues tag: component Issues or improvements associated with a given component in the theme kind: enhancement New feature or request labels Apr 18, 2024
This should fix pydata#1735, it is complementary to pydata#1564 but with a different
approach.

Instead of generating the same navbar as the fullpage width one, it
generate one with no dropdowns, with this, any css that tries to make
downtown looks like normal links in html becomes unnecessary.

Co-authored-by: gabalafou <[email protected]>
@Carreau
Copy link
Collaborator Author

Carreau commented Apr 23, 2024

unrelated type change removed and pulled in their own PR.

@Carreau
Copy link
Collaborator Author

Carreau commented Apr 23, 2024

BTW, every time I push the workflows re-need approval are the workflow settings a bit too agressive ? I though approval were only a one-time thing.

Carreau added a commit to Carreau/pydata-sphinx-theme that referenced this pull request Apr 23, 2024
On top of  pydata#1771, should fix pydata#1773 (once fininished)

This try to split the function into two parts: one that wrangle the
data,  one that generate html.

The generating the data part should be cacheable, while the other can be
made more flexible to be reusable in different part of the theme
(typically generating or not dropdowns in the sidebar or navbar).
@gabalafou
Copy link
Collaborator

BTW, every time I push the workflows re-need approval are the workflow settings a bit too agressive ? I though approval were only a one-time thing.

That's a good question. I don't have access to the workflow settings. @drammock do you?

@gabalafou
Copy link
Collaborator

In the meantime, I do have permission to approve and run workflows for a particular PR, so I went ahead and re-ran the workflows on this PR

@drammock
Copy link
Collaborator

That's a good question. I don't have access to the workflow settings. @drammock do you?

I just gave @Carreau write access to this repo, which should make the CIs run automatically on his PRs in future. Then I remembered that we usually have a process for this --- opening a PR nominating someone to have access, giving folks time to weigh in. Sorry for subverting the process!

I can still open that PR if folks want, and revoke access if @Carreau gets vetoed, WDYT? FWIW both @trallard and I know @Carreau personally and I trust him to not do anything dangerous here so if folks want to skip the formal vote that's also OK with me.

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.

How hard would it be to add a test for this? I'm thinking a test that checks that the sidebar renders the nav without the dropdown markup, whereas the header renders it with.

@Carreau
Copy link
Collaborator Author

Carreau commented Apr 25, 2024

I just gave @Carreau write access to this repo, which should make the CIs run automatically on his PRs in future. Then I remembered that we usually have a process for this --- opening a PR nominating someone to have access, giving folks time to weigh in. Sorry for subverting the process!

Thanks, much appreciated. and it's ok if you wish to remove me.

How hard would it be to add a test for this? I'm thinking a test that checks that the sidebar renders the nav without the dropdown markup, whereas the header renders it with.

I'm not too sure, I'll try to understand pytest-regression and how to select the right things in the sidebar.

Carreau added a commit to Carreau/pydata-sphinx-theme that referenced this pull request May 2, 2024
On top of  pydata#1771, should fix pydata#1773 (once fininished)

This try to split the function into two parts: one that wrangle the
data,  one that generate html.

The generating the data part should be cacheable, while the other can be
made more flexible to be reusable in different part of the theme
(typically generating or not dropdowns in the sidebar or navbar).
@Carreau
Copy link
Collaborator Author

Carreau commented May 2, 2024

Ok, there was no test for the nav of the sidebar, I think I added one in the right place. This should also take care of #1799

@drammock
Copy link
Collaborator

drammock commented May 2, 2024

@gabalafou you requested changes here, can you take another look and merge if satisfied?

@Carreau Carreau requested a review from gabalafou May 7, 2024 12:20
Carreau added a commit to Carreau/pydata-sphinx-theme that referenced this pull request May 7, 2024
On top of  pydata#1771, should fix pydata#1773 (once fininished)

This try to split the function into two parts: one that wrangle the
data,  one that generate html.

The generating the data part should be cacheable, while the other can be
made more flexible to be reusable in different part of the theme
(typically generating or not dropdowns in the sidebar or navbar).
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.

Sorry for the delay, thanks for adding a test!

@drammock drammock merged commit a31db4c into pydata:main May 8, 2024
19 checks passed
@Carreau
Copy link
Collaborator Author

Carreau commented May 13, 2024

Thanks !

Carreau added a commit to Carreau/pydata-sphinx-theme that referenced this pull request May 14, 2024
On top of  pydata#1771, should fix pydata#1773 (once fininished)

This try to split the function into two parts: one that wrangle the
data,  one that generate html.

The generating the data part should be cacheable, while the other can be
made more flexible to be reusable in different part of the theme
(typically generating or not dropdowns in the sidebar or navbar).
ivanov pushed a commit to ivanov/pydata-sphinx-theme that referenced this pull request Jun 5, 2024
* Do not generate dropdown in sidebar.

This should fix pydata#1735, it is complementary to pydata#1564 but with a different
approach.

Instead of generating the same navbar as the fullpage width one, it
generate one with no dropdowns, with this, any css that tries to make
downtown looks like normal links in html becomes unnecessary.

Co-authored-by: gabalafou <[email protected]>

* apply review

* Add failing test for dropdown sidebar

---------

Co-authored-by: gabalafou <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: enhancement New feature or request tag: component Issues or improvements associated with a given component in the theme tag: CSS CSS and SCSS related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Some links in the left mobile sidebar act like links in dropdown
4 participants