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

Add display value of bar chart #114

Merged
merged 2 commits into from
Sep 12, 2018
Merged

Conversation

dgdblank
Copy link
Contributor

Allow users to optionally show the values of the bar in the bar chart.

  • Includes text class for changing the styles
  • Works for both horizontal and vertical bars, but only displays to the right of horizontal bars
  • Includes formatting function prop for changing how the values display

@dandelany
Copy link
Contributor

dandelany commented Sep 12, 2018

Thanks for the contribution, @dgdblank ! I haven't had time to do a full review & test yet, but just a few notes:

  • For the sake of consistency with other components (eg. the axis components), I think we should use the word "label" instead of "text", eg. labelDistance, labelFormat etc. rather than textDistance, textFormat
  • Also for consistency - for booleans which determine whether or not something is shown, we tend to use the prefix show rather than display (again, see axis components for example). So maybe displayValue -> showLabel (or showBarLabel?)
  • We generally only use all-caps variable names for constants (usually strings) which are used across the library - so RECT and TEXT in Bar.js can just be rect and text
  • Longer-term, I think we may want to implement a more generalized solution to labeling values shown on XYPlots, something like a separate XYValueLabel or even XYValueLabelCollection component(s), rather than combining it into the Bar component. This would allow us to use the same label component for multiple types of plots, instead of just bars, and would fit more with our philosophy of composition & having each component "do one thing well". However, getting all of the props/options we'll want for generalized labels will be quite a bit of work, so I think this is a good interim solution until we figure out our long-term roadmap for value labels.

Thanks again for your work on this! Hope this doesn't seem too nitpicky but I think it's valuable for us to ensure naming consistency across the library, to reduce developer confusion whenever possible.

-d

src/Bar.js Outdated
};
static defaultProps = {
x: 0,
y: 0,
thickness: 8,
className: "",
style: {}
style: {},
textDistance: 24
Copy link
Contributor

Choose a reason for hiding this comment

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

is this used?

@dgdblank
Copy link
Contributor Author

@krissalvador27 whoops, fixed.

@krissalvador27
Copy link
Contributor

krissalvador27 commented Sep 12, 2018

Like @dandelany said I agree that a long term solution around some smart HOC or something of that flavor could be implemented that can allow someone to decorate bars, scatterplot points etc in the future with provided coordinates (among other props/options). Will merge though since this is a good interim solution!

@krissalvador27 krissalvador27 merged commit 1d3ca03 into spotify:master Sep 12, 2018
@dandelany
Copy link
Contributor

👏 👏 thanks @krissalvador27 @dgdblank

@dgdblank dgdblank deleted the bar-labels branch September 12, 2018 22:17
install pushed a commit that referenced this pull request Feb 25, 2020
* Add display value of bar chart

* Change names on Bar from text to labels
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