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

Canavandl/1441 colorbar #4916

Closed
wants to merge 18 commits into from
Closed

Canavandl/1441 colorbar #4916

wants to merge 18 commits into from

Conversation

canavandl
Copy link
Contributor

@canavandl canavandl commented Aug 1, 2016

(ninja edit) Note: this branch is branched from #4851 and should only be merged after the parent.

Proof-of-concept color bar

  • issues: fixes Colorbar Axis #1441
  • tests added / passed
  • release document entry (if new feature or API change)

The code is a mess (lots of copy pasta from axis.coffee and hardcoded values), but serves to generate a POC color bar. (this uses a LogColorMapper, but Linear looks identical except for the tick spacing).

screen shot 2016-08-01 at 6 36 27 pm

@canavandl
Copy link
Contributor Author

canavandl commented Aug 1, 2016

Apart from code issues (including the floating tick above the color bar), are there any visual components that I'm missing. Specifically:

  • Should a colorbar have a title somewhere? Above or to the right of the ticks?
    • Maybe on top? I'm not sure it's necessary though.
  • I could enclose the entire color bar in a rect with configurable fill props. Is that worthwhile?
    • I think I'm +1 on this and will try to prototype. Something with some nice padding around the non-scale side of the image.
  • Anything else?

@canavandl
Copy link
Contributor Author

POC bounding rect of color bar (default color would probably be None and padding would be tweaked):

screen shot 2016-08-01 at 7 05 09 pm

@birdsarah
Copy link
Member

birdsarah commented Aug 2, 2016

Should a colorbar have a title somewhere? Above or to the right of the ticks?

Yes (optionally) - if the ticks are on the right, the default position should be on the top left - left aligned with the actual color bar (not aligned with the text)

I could enclose the entire color bar in a rect with configurable fill props. Is that worthwhile?

I think it's fine to leave that for later

Anything else?

I think this should have the same padding, margin etc properties as legend, specifically as proposed by @almarklein here: #4526 - so the same properties as legend will have!

@canavandl canavandl mentioned this pull request Aug 2, 2016
3 tasks
@canavandl
Copy link
Contributor Author

Now the location/orientation properties are correctly implemented (though still ugly):

screen shot 2016-08-02 at 3 33 47 pm

from .renderers import GuideRenderer, GlyphRenderer
from .tickers import Ticker, BasicTicker

class Legend(GuideRenderer):
Copy link
Contributor

Choose a reason for hiding this comment

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

This change needs an explanation (should have been done in the initial commit). There might be unforeseen consequences of this change across code base. This is also why one should refrain from using "refactoring" near such changes, as this changes both APIs and functionality.

Copy link
Contributor Author

@canavandl canavandl Aug 2, 2016

Choose a reason for hiding this comment

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

The refactoring of Legend into a subclass of GuideRenderer is isolated in PR #4851 and this branch is merged off of the #4851 parent. I should have mentioned that in the initial comment (I'll add it now).

@canavandl canavandl mentioned this pull request Aug 6, 2016
3 tasks
@canavandl canavandl closed this Aug 6, 2016
@canavandl canavandl deleted the canavandl/1441_colorbar branch August 6, 2016 01:17
@canavandl
Copy link
Contributor Author

superceded by #4931

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.

Colorbar Axis
3 participants