-
-
Notifications
You must be signed in to change notification settings - Fork 18.6k
WEB: Refine PDEP presentation #58791
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
WEB: Refine PDEP presentation #58791
Conversation
h2:hover a.headerlink { | ||
opacity: 1; | ||
transition: opacity 0.5s; | ||
} | ||
h3:hover a.headerlink { | ||
opacity: 1; | ||
transition: opacity 0.5s; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should be able to combine these two, but wasn't able to figure out the syntax.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's
h2:hover a.headerlink, h3:hover a.headerlink {
...
}
You can avoid repeating the content, but I don't think you can avoid repeating a.headerlink or other things in the label.
body = markdown.markdown( | ||
content, extensions=context["main"]["markdown_extensions"] | ||
) | ||
if "pdeps/" in fname: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because of the use of TocExtension
below, we can't use the usual yaml. Not sure if there is a work around or a better approach here.
/preview |
Website preview of this PR available at: https://pandas.pydata.org/preview/pandas-dev/pandas/58791/ |
PDEP6 shows code highlighting. |
@rhshadrach thanks a lot for looking into this! I certainly think this is a nice improvement, and if we don't have agreement on using sphinx instead, we should definitely merge this one. But if you ask my honest opinion, I personally still think #51467 gives the better result. Concrete feedback on the changes here:
|
Personal preference, but I think the "On this page" sidebar (from @jorisvandenbossche's PR) is cleaner and resembles the docs more than the table of contents here. (I guess that's a benefit of using Sphinx?) |
This pull request is stale because it has been open for thirty days with no activity. Please update and respond to this comment if you're still interested in working on this. |
/preview |
@rhshadrach should we merge this? |
/preview |
Website preview of this PR available at: https://pandas.pydata.org/preview/pandas-dev/pandas/58791/ |
/preview |
Website preview of this PR available at: https://pandas.pydata.org/preview/pandas-dev/pandas/58791/ |
/preview |
Website preview of this PR available at: https://pandas.pydata.org/preview/pandas-dev/pandas/58791/ |
Thanks @rhshadrach, very nice. Maybe the ecosystem can also benefit from this, I'll create an issue. |
This PR still suffered from the height issue that @jorisvandenbossche pointed out above. |
It works well in my browser, visualization is fine, and links work perfect. I use Brave on Linux. Not sure if I can't see the problem, or just works well for me. Feel free to open an issue with the exact problem, if I can reproduce it happy to take a look, otherwise someone else will probably fix it if you label it as good first issue. |
Weird I'm not seeing the same. Perhaps I was testing an old link when I last looked at this. But now I'm noticing that the Table of Contents is gone. |
Ah, it's only on the Implemented pages. |
Only thing I was thinking is whether it's been this PR that decreased the width of the content of the website. Maybe just my perception, but I think it was wider (maybe it became narrower before). But the tables of content look fine everywhere I checked, also in the implemented section, at list the first one: https://pandas.pydata.org/pdeps/0004-consistent-to-datetime-parsing.html |
transition: opacity 0.5s; | ||
} | ||
.container { | ||
max-width: 100ch; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, this is what's making the page narrower. Seems intentional, do you think it looks better? I personally prefer the default bootstrap sizing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The intention here was to decrease it only on PDEP pages. Looks like that's not what's happening. On pages that are more like articles, I do indeed prefer a more narrow text (this is pretty standard in academia).
doc/source/whatsnew/vX.X.X.rst
file if fixing a bug or adding a new feature.Alternative to #51467. I believe this address all items in the linked issue. The main benefit in not switching to publishing PDEPs in sphinx is that it then matches the style of the rest of the webpage.
This is just a rough version to demonstrate what's possible, some refinements (highlighted below) need to be made if we are to merge this.