Skip to content

Conversation

loftiskg
Copy link
Contributor

@loftiskg loftiskg commented Nov 2, 2019

Reference Issues/PRs

Fixes #12941

What does this implement/fix? Explain your changes.

Added CSS to prevent line breaking in the middle of a word in a parameter description.

Any other comments?

@skubatur co-authored this request with me.

Copy link
Member

@TomDLT TomDLT left a comment

Choose a reason for hiding this comment

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

Thanks for the pull-request !
A small comment:

@@ -823,6 +823,10 @@ div.body img {
height: unset!important; /* Needed because sphinx sets the height */
}

div.body dd > p {
Copy link
Member

@TomDLT TomDLT Nov 2, 2019

Choose a reason for hiding this comment

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

Your change is not located in doc/themes/scikit-learn/static/nature.css_t as suggested in #12941 (comment).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that previous comment sort of threw me for a loop.

I think there was a change to which stylesheet the documentation uses since that last comment. doc/themes/scikit-learn-modern/static/css/theme.css was the only stylesheet that I could change that would eventually be linked with the page in question.

I did a quick search, and I think it happened in this merge: #14849

Copy link
Member

Choose a reason for hiding this comment

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

yeah @thomasjpfan rewrote the frontend / css

@amueller
Copy link
Member

amueller commented Nov 2, 2019

we should wait for Circle to render this so we can confirm it fixed the issue.

@amueller
Copy link
Member

amueller commented Nov 2, 2019

hm ok Circle will probably not catch up today though :(

@cmarmo
Copy link
Contributor

cmarmo commented Nov 13, 2019

Hi @loftiskg, the checks are failing for temporary issues on azure.
Could you please synchronize with upstream and push? Your PR is almost done... :)
Thanks!

@loftiskg
Copy link
Contributor Author

@cmarmo sure thing!

@cmarmo
Copy link
Contributor

cmarmo commented Nov 14, 2019

@amueller, @TomDLT , check passed! :)
When you have some time, let us know if all is good for you. Thanks!

Copy link
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

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

LGTM

@TomDLT TomDLT merged commit 7042019 into scikit-learn:master Nov 14, 2019
@TomDLT
Copy link
Member

TomDLT commented Nov 14, 2019

Thanks @loftiskg !

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.

dash added to documentation changes options
5 participants