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 Distribution #435

Merged
merged 7 commits into from Aug 6, 2019

Conversation

@bpierre
Copy link
Member

commented Aug 3, 2019

image

Distribution can be used to visualize the distribution of a given value. It is often used inside the Box component, but can be used elsewhere too.

@bpierre bpierre requested a review from sohkai Aug 3, 2019

@bpierre bpierre referenced this pull request Aug 3, 2019

@bpierre bpierre changed the title Distribution Add Distribution Aug 3, 2019

@bpierre bpierre requested a review from AquiGorka Aug 5, 2019

@AquiGorka
Copy link
Member

left a comment

Awesome.
Question: Does Box render a section tag? If not, it should no?
In that case, if Distribution is used inside('Box') should the section container change for something else?

@bpierre

This comment has been minimized.

Copy link
Member Author

commented Aug 6, 2019

Question: Does Box render a section tag? If not, it should no?

It does, when a heading is set:

<div
as={heading ? 'section' : 'div'}

In that case, if Distribution is used inside('Box') should the section container change for something else?

I don’t think it would be needed. Having:

section
  h1 (Box heading)
  section
    h1 (Distribution heading)

Is equivalent of having:

section
  h1 (Box heading)
  h2 (Distribution heading)

When used elsewhere, or in a Box without heading, it will be relative to the closest sectioning content.

@sohkai

sohkai approved these changes Aug 6, 2019

Copy link
Member

left a comment

Let's add a gallery page 😉.

Otherwise a few small things about the API, but it looks good as well!

typeof value === 'string' ? value : `Item ${index + 1}`
}: ${percentage}%`
},
renderLegendItem: ({ value, percentage, index }) => {

This comment has been minimized.

Copy link
@sohkai

sohkai Aug 6, 2019

Member

Maybe we should use name instead of value?

This comment has been minimized.

Copy link
@bpierre

bpierre Aug 6, 2019

Author Member

Yes I don’t like it much either… but value can be anything, not only a string. For example, we could imagine that every item is an object with a name and an icon.

But I’m thinking now that we could move from:

<Distribution
  values={[
    { value: 'Bananas', percentage: 37 },
    { value: 'Apples', percentage: 22 },
    { value: 'Cherries', percentage: 15 },
    { value: 'Oranges', percentage: 12 },
    { value: 'Grapefruits', percentage: 12 },
    { value: 'Rest', percentage: 2 },
  ]}
/>

To:

<Distribution
  items={[
    { item: 'Bananas', percentage: 37 },
    { item: 'Apples', percentage: 22 },
    { item: 'Cherries', percentage: 15 },
    { item: 'Oranges', percentage: 12 },
    { item: 'Grapefruits', percentage: 12 },
    { item: 'Rest', percentage: 2 },
  ]}
/>

And then pass it to renderLegendItem:

renderLegendItem: ({ item, percentage, index }) => {

It would also make value available for a future version: I decided to have percentages rather than number values for now, but I would like to calculate them like stakesPercentages() does, by passing a list of values and a maximum number of items we want to render (grouping the extra items in “rest” automatically). It would require to be adapted a bit so that BN.js values are supported without bundling the library into aragonUI (to save 10k). I think the API of BN.js is stable enough to take that risk?

Another option could be to convert BN.js objects into BigInt with a babel transform for non-supported browsers (which would use jsbi − 8k), so that we are ready for the day Safari starts supporting BigInt.

This comment has been minimized.

Copy link
@sohkai

sohkai Aug 6, 2019

Member

I like item more than value, but mainly value is confusing because we only use it for a label.

It would also make value available for a future version: I decided to have percentages rather than number values for now, but I would like to calculate them

I like the simplicity of the current approach, and I feel like this concern could be lifted outside of this component. The current ability to render more than 100% total is actually a feature in my eyes.

This comment has been minimized.

Copy link
@bpierre

bpierre Aug 6, 2019

Author Member

I like the simplicity of the current approach, and I feel like this concern could be lifted outside of this component.

Maybe yes… I just feel that passing a list of values would be the easiest, rather than a having to calculate a list of percentages which can get pretty complex, like in stakesPercentages(). We could go for a default way of doing this calculation, and in cases where users want something diffferent, they’ll always be able to do it outside and pass their percentage manually. In other words, the component would still support the current mode, but also have a “simple mode” that doesn’t require to calculate percentages.

The current ability to render more than 100% total is actually a feature in my eyes.

I started by warning when the total doesn’t equal 100%, but then I remembered cases like 33% - 33% - 33%, so the component now only warns if the total is greater than 100%. Do you think we should remove it as well? Do you have a case in mind where having a total greater than 100% would be needed?

src/components/Distribution/Distribution.js Outdated Show resolved Hide resolved

bpierre added some commits Aug 6, 2019

@bpierre bpierre merged commit f82b065 into newstyle Aug 6, 2019

2 of 5 checks passed

continuous-integration/travis-ci/push The Travis CI build could not complete due to an error
Details
License Compliance FOSSA is analyzing this commit
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
WIP Ready for review
Details
license/cla Contributor License Agreement is signed.
Details

@delete-merged-branch delete-merged-branch bot deleted the newstyle-distribution branch Aug 6, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.