Skip to content

DOC: point align-ylabel demo to new align-label functions #11424

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 1 commit into from
Jun 21, 2018

Conversation

jklymak
Copy link
Member

@jklymak jklymak commented Jun 12, 2018

@jklymak jklymak added this to the v2.2-doc milestone Jun 12, 2018
@jklymak jklymak force-pushed the doc-align-ylabels branch 2 times, most recently from 097ad89 to 2fecd9b Compare June 12, 2018 17:57
Copy link
Member

@ImportanceOfBeingErnest ImportanceOfBeingErnest left a comment

Choose a reason for hiding this comment

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

Many examples indeed lack proper cross linking to similar/opposite/related cases. This is a step in the right direction.

@ImportanceOfBeingErnest
Copy link
Member

tbh I find the two big boxes on top a bit... too much. How did the Note box get introduced anyways? Can we get rid of that? I mean the page should start with its title, not with a note.

image

@jklymak
Copy link
Member Author

jklymak commented Jun 12, 2018

I didn't introduce that! sphinx-gallery maybe?

EDIT: WRT the second blue box, I think its justified - the new methods are much easier for most uses...

@ImportanceOfBeingErnest
Copy link
Member

I'm proposing to remove that box.

@timhoffm
Copy link
Member

I’d rather have See also sections below the code. Having it up the is like saying “I’ll show you how to do alignment, but first you should look at these alternative ways.

Then again the referenced method is the simpler more standard case, which should come first.

The best solution would be to rewrite the example: First show a simple ˋalign_ylabel()ˋ example, then show the current code in case someone needs more control.

But I do not necessarily require this as part of this PR. Mentioning ˋalign_ylabel()ˋ is an improvement already.

@jklymak
Copy link
Member Author

jklymak commented Jun 17, 2018

Well ok fair enough. On phone and can’t label but I agree that combining the examples is a better solution. So please don’t merge until I do that.

@jklymak jklymak force-pushed the doc-align-ylabels branch 3 times, most recently from 7807912 to 9adb067 Compare June 18, 2018 16:50
@jklymak jklymak force-pushed the doc-align-ylabels branch 2 times, most recently from 3c27acc to 0edc77c Compare June 19, 2018 02:36
@jklymak jklymak force-pushed the doc-align-ylabels branch from 0edc77c to 07501bd Compare June 19, 2018 02:37
@jklymak
Copy link
Member Author

jklymak commented Jun 21, 2018

@timhoffm are the changes ok? If so, we can merge...

@timhoffm timhoffm merged commit b078643 into matplotlib:master Jun 21, 2018
@lumberbot-app
Copy link

lumberbot-app bot commented Jun 21, 2018

There seem to be a conflict, please backport manually

@jklymak
Copy link
Member Author

jklymak commented Jun 21, 2018

Not a big deal to bother w/ backport... Thanks @timhoffm!

@jklymak jklymak deleted the doc-align-ylabels branch June 21, 2018 17:22
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.

4 participants