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

[86by6f2gg][d3-chart] added cigarette pack chart #1477

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

msereniti
Copy link
Contributor

Motivation and Context

This PR adds new chart type.

How has this been tested?

Tests are to be added

Screenshots (if appropriate):

Screenshot 2024-06-28 at 13 58 02

Types of changes

  • Bug fix (non-breaking change which fixes an issue).
  • New feature (non-breaking change which adds functionality).
  • Breaking change (fix or feature that would cause existing functionality to not work as expected).
  • Nice improve.

Checklist:

  • My code follows the code style of this project.
  • I have updated the documentation accordingly or it's not required.
  • Unit tests are not broken.
  • I have added changelog note to corresponding CHANGELOG.md file with planned publish date.
  • I have added new unit tests on added of fixed functionality.

@msereniti msereniti changed the title [d3-chart] added distribution bar chart base [86by6f2gg][d3-chart] added distribution bar chart base Jun 28, 2024
@msereniti msereniti marked this pull request as ready for review June 28, 2024 19:24
@msereniti msereniti changed the title [86by6f2gg][d3-chart] added distribution bar chart base [86by6f2gg][d3-chart] added cigarette pack chart Jul 4, 2024
@msereniti msereniti requested a review from ilyabrower July 4, 2024 15:12
Copy link
Contributor

@j-mnizhek j-mnizhek left a comment

Choose a reason for hiding this comment

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

I see that this chart still has border-radius. Can we remove it? 🙏

@j-mnizhek
Copy link
Contributor

@sheila-semrush Sheila, can you please check only documentation for CigarettePack chart when you have time?

sheila-semrush

This comment was marked as duplicate.

Copy link
Contributor

@sheila-semrush sheila-semrush left a comment

Choose a reason for hiding this comment

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

A lot of work done, thanks!!!
I left several suggestions

@@ -54,10 +54,16 @@ const group = {
type: 'charts',
},
cigaretteChart: {
title: 'Cigarette chart',
title: 'Cigarette bar chart',
Copy link
Contributor

Choose a reason for hiding this comment

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

hmm the cigarette disappeared (seems to be connected to its renaming) and the cigarette pack is not on the showcase page for me. Although I did run install & build. Should I run something else? Or is something broken?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's a bug, because the same was with the cigarette chart. It's planned to be fixed of course.

website/docs/data-display/cigarette-pack/cigarette-pack.md Outdated Show resolved Hide resolved
website/docs/data-display/cigarette-pack/cigarette-pack.md Outdated Show resolved Hide resolved
website/docs/data-display/cigarette-pack/cigarette-pack.md Outdated Show resolved Hide resolved
plotHeight={450}
onClickBar={(barIndex) => alert(`Bar ${barIndex} clicked`)}
/>
<Plot data={data} scale={[xScale, yScale]} width={width} height={height}>
Copy link
Contributor

Choose a reason for hiding this comment

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

did we intentionally put 2 charts here? if yes, maybe worth splitting this example into 2 code snippets? (we did this in Tag, there's 1 example of tag grouping, but it has 2 code snippets with subheaders for clarity)


**Cigarette pack bar chart** visualizes the distribution of values by category for a part-to-whole comparison.

## Usage
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it might be useful to put somewhere here in the beginning a little paragraph with an illustration of the horizontal bar chart (the variation that's similar to the cigarette pack), with something like "if you're thinking about this, then go there" (and a link to the horizontal bar chart page)

because I feel people might go to this page, remembering just visually what they need, but not remembering the exact name, go through the illustrations (because who reads), see that there's no variant of the chart with labels to the left of the bars, and think that there's just no documentation for that, or we removed that feature

Copy link
Contributor

Choose a reason for hiding this comment

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

and maybe even acknowledge in the text that yes, these 2 things look very similar, but they're on different pages. This might make it much clearer for documentation users

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe an illustration with both charts clearly labelled would be useful

- Each category's value needs labeling.
- Showing category distribution in order.

**Avoid horizontal bars when:**
**Use [Cigarette pack bar chart](/data-display/cigarette-pack/cigarette-pack) instead of Horizontal bar chart when:**
Copy link
Contributor

Choose a reason for hiding this comment

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

same comment as for the cigarette pack page, I'd recommend adding a little explanation that these 2 charts are really only different in labels positioning, and if you were thinking of this (illustration), then it's the cigarette pack
maybe an illustration with both charts clearly labelled would be useful

Copy link
Contributor

Choose a reason for hiding this comment

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

They are actually differs a little bit in when they should be used, but overall I agree. I was doubting about adding something more straightforward in the text. Will add. It could be helpful indeed.

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.

None yet

4 participants