Skip to content
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

Start with legends, and less references #40

Merged
merged 1 commit into from
Jun 2, 2019
Merged

Conversation

Ploppz
Copy link
Contributor

@Ploppz Ploppz commented Apr 20, 2019

I am planning to work on the ideas in #34 and I had some changes in plotlib so I thought it could be a good idea to do a PR with those changes now, so that the next PR which builds on these changes is easier to read.

First and foremost I started implementing legends.
Notice that this is only implemented for Line so far. But in the following PR it will be implemented for Function and Scatter as well.

Secondly, I removed some & and replaced them with Box - better interface.

And also give the programmer possibility to specify max number of x and y ticks.

@milliams
Copy link
Owner

milliams commented Jun 2, 2019

Hi @Ploppz,

This look good to me. I like the reduction in references and lifetimes which help simplify the API.

For the legends, I don't think we really want each representation to be specifying the SVG output of their legend, but rather should provide the abstract information needed (marker colour, shape etc. and label text) to to the View so that it can be in control of creating all the legend segments. In general, I'd like to keep Representations as abstract as possible with respect to rendering and treat them more as a holder for the processed information from the data source.

I'm happy to merge this since it's still all moving in the right direction for us.

Thanks a lot.

@milliams milliams merged commit 027bd14 into milliams:master Jun 2, 2019
@milliams milliams added this to the 0.5.0 milestone Mar 14, 2020
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.

2 participants