Skip to content

Conversation

@zuggernautt
Copy link
Contributor

@zuggernautt zuggernautt commented Feb 8, 2022

Reference Issues/PRs

part of #22406

What does this implement/fix? Explain your changes.

Any other comments?

Copy link
Member

@lesteve lesteve left a comment

Choose a reason for hiding this comment

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

In this case it feels like this could be turned into titles. If you apply my suggestions it should work.

In other words, I find it slightly weird to have small text elements like this before a code cell and think a title would be more appropriate here:
image

This is the kind of things that others may not have the same opinion ...

Also I added the issue reference #22406 in your PR description to make it easier to track the advancement on this issue. If you ever open another PR tackling part of this issue, it would be great if you can remember to do it!

@zuggernautt
Copy link
Contributor Author

In this case it feels like this could be turned into titles. If you apply my suggestions it should work.

In other words, I find it slightly weird to have small text elements like this before a code cell and think a title would be more appropriate here: image

This is the kind of things that others may not have the same opinion ...

Also I added the issue reference #22406 in your PR description to make it easier to track the advancement on this issue. If you ever open another PR tackling part of this issue, it would be great if you can remember to do it!

Thank you for your suggestions, it was my first pr.

@lesteve
Copy link
Member

lesteve commented Feb 9, 2022

@lesteve lesteve mentioned this pull request Feb 9, 2022
47 tasks
@ArturoAmorQ
Copy link
Member

I am not sure about the Author/License block appearing as commented code, can we turn it into markdown?

@lesteve
Copy link
Member

lesteve commented Feb 9, 2022

I am not sure about the Author/License block appearing as commented code, can we turn it into markdown?

I don't think it is worth it, I agree it looks a bit weird on its own like this but 🤷, maybe we can move it down in the first code cell

@ArturoAmorQ
Copy link
Member

@lesteve
Copy link
Member

lesteve commented Feb 9, 2022

Let's merge this this is a good improvement already, thanks a lot!

@lesteve lesteve merged commit b260100 into scikit-learn:main Feb 9, 2022
ilivans pushed a commit to ilivans/scikit-learn that referenced this pull request Feb 10, 2022
thomasjpfan pushed a commit to thomasjpfan/scikit-learn that referenced this pull request Mar 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants