Skip to content

DOC [PST] tune FAQ page styling #28448

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

Merged
merged 6 commits into from
Apr 10, 2024

Conversation

Charlie-XIAO
Copy link
Contributor

Please note that this PR targets the new_web_theme branch!

Towards #28084. This PR:

  • Removes the .. toctree:: in the page so that the question headings do not become (meaningless, in the sense that we have the secondary sidebar) links.
  • Makes the questions (h3 headings) rubric-like; this is personal taste so please let me know if maintainers think the original style is better.
Current This PR
image image

Copy link

github-actions bot commented Feb 18, 2024

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

Generated for commit: b0f84bb. Link to the linter CI: here

@Charlie-XIAO Charlie-XIAO marked this pull request as ready for review February 18, 2024 03:05
@Charlie-XIAO
Copy link
Contributor Author

Charlie-XIAO commented Feb 22, 2024

Just to confirm @ogrisel, do you mean that we should keep the .. toctree:: at the top of the page while removing the backlinks of the headers? (This could be a bit difficult and I need to do further investigations.)

@adrinjalali
Copy link
Member

ping @ogrisel

@ogrisel
Copy link
Member

ogrisel commented Mar 11, 2024

Just to confirm @ogrisel, do you mean that we should keep the .. toctree:: at the top of the page while removing the backlinks of the headers? (This could be a bit difficult and I need to do further investigations.)

Yes. But if that's too cumbersome/hackish to implement let's keep it the way it is in this PR.

Copy link
Member

@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

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

I think I would have preferred to keep the toctree at the top of the page though (as explained in #28448 (review)), but I am fine with the current state of this PR. I let you and the second reviewer decide :)

@Charlie-XIAO
Copy link
Contributor Author

Thanks for the clarification @ogrisel. AFAIK there is not an option of the .. toctree:: directive that let us easily remove the backlinks. Two possible ways that I can think of are: (1) implement our own directive and there should be some way to resolve the in-page toctree; (2) use a post transform to steal the toctree built by pydata-sphinx-theme and put it in the main page. But as you said they might be too hackish just for this purpose.

@adrinjalali
Copy link
Member

I think I do miss the ToC on top of the page. I personally wouldn't mind having the ToC on top, AND have the usual pydata theme navbar on the right side. Would that be okay?

@Charlie-XIAO
Copy link
Contributor Author

Charlie-XIAO commented Mar 12, 2024

@adrinjalali Yes that's perfectly okay.

The tradeoff is in fact "keeping the TOC at the top of the page" will cause "backlinks to be generated for the titles which does not look good".

I however made a mistake in the previous comment: the statement there is no trivial way to remove backlink styles is incorrect. In CSS there are pointer-events: none that disables any click events and text-decoration: none that removes link underlines. (Earlier I put those on the h2 and h3 elements (stupid mistake) and found that the backlink styles are not reverted, but I should have put those styles on the anchor elements inside them. 😭)

Well anyways, I think the latest version would resolve all the mentioned problems. Also ping @ogrisel if you want to take another look. https://output.circle-artifacts.com/output/job/f3d31074-de89-4934-8c65-c1c8bbe171c2/artifacts/0/doc/faq.html

@ogrisel
Copy link
Member

ogrisel commented Mar 14, 2024

I think text-decoration: none is enough. No need to remove the actual backlinks clicking behavior.

Copy link
Member

@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

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

LGTM, with or without the pointer-events rule.

@Charlie-XIAO
Copy link
Contributor Author

Charlie-XIAO commented Mar 14, 2024

I think text-decoration: none is enough. No need to remove the actual backlinks clicking behavior.

On second thought I agree. Now I only kept text-decoration: none and removed the color and pointer-events rules.

https://output.circle-artifacts.com/output/job/335dffa0-d8c4-4d70-a396-d156d96c83e4/artifacts/0/doc/faq.html

@adrinjalali adrinjalali merged commit 3852926 into scikit-learn:new_web_theme Apr 10, 2024
@Charlie-XIAO Charlie-XIAO deleted the pst-faq branch April 10, 2024 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants