Skip to content

DOC Tutorial structure and layout #18261

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 15 commits into from
Aug 29, 2020

Conversation

cmarmo
Copy link
Contributor

@cmarmo cmarmo commented Aug 26, 2020

Reference Issues/PRs

Towards #18257, uses #17865

What does this implement/fix? Explain your changes.

Fix unresponsive layout and general rendering of the tutorial.
Remove the 'Finding help' section.

Additional comments

I haven't addressed the TOC issue, I think this should be solved in a consistent way over the entire website (see also #16951)

Copy link
Member

@NicolasHug NicolasHug left a comment

Choose a reason for hiding this comment

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

Thanks @cmarmo, the rendering does look better.

I'm not super experienced here, but I'm a little bit concerned about injecting raw html into the .rst. I guess that's fine in the about page because it won't change too much, but here we would have to handle the html code on top of the narrative docs which may become unfeasible.

Is there a way to handle this purely in sphinx? Maybe using the figclass attribute of the figure? https://docutils.sourceforge.io/docs/ref/rst/directives.html#figure (again, I don't have a clear idea of what would work here)

If really that sounds too complex, then I'd suggest to drop the side-by-side layout and just have pictures and code aligned vertically.

@cmarmo
Copy link
Contributor Author

cmarmo commented Aug 26, 2020

Fine, I have removed all raw html calls. The layout is quite limited, but the the content will soon be modified so this is not an issue.
I'm quite skeptical about the maintainability comment, because I don't find the website, as of today, easily maintainable in general.
I really feel like this pull request is now quite unuseful. I would appreciate a quick merge or just a rejection. Thanks.

Copy link
Member

@NicolasHug NicolasHug left a comment

Choose a reason for hiding this comment

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

Thanks.

There seem to be some html left in supervised_learning.rst

I'm quite skeptical about the maintainability comment, because I don't find the website, as of today, easily maintainable in general.

I mean that mixing narrative docs + html is something that will hurt maintainability in the long run. As of today, we have zero html manually injected in our narrative docs, and I think that's for the better. I wasn't talking about the website. I definitely believe you if you say it's hard to maintain.

I would appreciate a quick merge or just a rejection. Thanks.

I'm doing my best by reviewing this @cmarmo. Thanks for your work so far.

EDIT sorry pressed enter too soon

@cmarmo
Copy link
Contributor Author

cmarmo commented Aug 26, 2020

There seem to be some html left in supervised_learning.rst

Right, sorry. Fixed.

I mean that mixing narrative docs + html is something that will hurt maintainability in the long run. As of today, we have zero html manually injected in our narrative docs, and I think that's for the better.

Understood, thanks for clarifying.

Copy link
Member

@NicolasHug NicolasHug left a comment

Choose a reason for hiding this comment

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

made a few comments, otherwise LGTM

Copy link
Member

@NicolasHug NicolasHug left a comment

Choose a reason for hiding this comment

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

thanks @cmarmo !

Looks much better already

Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
@thomasjpfan thomasjpfan merged commit bebb6d2 into scikit-learn:master Aug 29, 2020
@cmarmo cmarmo deleted the tutorial-struct-and-layout branch August 29, 2020 16:17
jayzed82 pushed a commit to jayzed82/scikit-learn that referenced this pull request Oct 22, 2020
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
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