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

Examples: Add pie and donut charts #88

Conversation

louismaximepiton
Copy link
Member

@louismaximepiton louismaximepiton commented Jan 24, 2024

Note: Please transform - [ ] into - (NA) in the description when things are not applicable

Related issues

NA

Description

Added an example showing a pie and a donut charts.

Questions still to be answered:

  • Do we make several pages for these two kinds of charts or only one is enough ? If not, remove the ods-donut-charts.png.
  • Not quite sure of the values I gave inside the examples. Need to be discussed or is it fine as a first draft ?
  • Do we need to update getThemeOptions accordingly ? For example we don't have axis on this kind of chart.

Motivation & Context

These type of charts are considered as basic and are missing inside the doc.

Types of change

  • New feature (non-breaking change which adds functionality)

Test checklist

Please check that the following tests projects are still working:

  • docs/examples
  • test/angular-ngx-echarts
  • test/angular-tour-of-heroes
  • test/html
  • test/react
  • test/vue
  • test/examples/bar-line-chart
  • test/examples/single-line-chart
  • test/examples/timeseries-chart

Copy link

netlify bot commented Jan 24, 2024

Deploy Preview for ods-charts ready!

Name Link
🔨 Latest commit 6ee3008
🔍 Latest deploy log https://app.netlify.com/sites/ods-charts/deploys/66a3443d0868c80008fefd9c
😎 Deploy Preview https://deploy-preview-88--ods-charts.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@jacques-lebourgeois jacques-lebourgeois self-requested a review March 1, 2024 16:59
@jacques-lebourgeois
Copy link
Member

Hi, I push 3 commits, 2 suggestion and one enhancement.

Available to discuss about them :)

Copy link
Member Author

@louismaximepiton louismaximepiton left a comment

Choose a reason for hiding this comment

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

I feel right with your changes, the display is great and as an alpha version it's fine to add some support to legend I think

docs/examples/index.js Outdated Show resolved Hide resolved
docs/examples/index.js Outdated Show resolved Hide resolved
@julien-deramond
Copy link
Contributor

#158 has been merged, please apply the same modifications here.

@jacques-lebourgeois
Copy link
Member

#158 has been merged, please apply the same modifications here

Commited

@louismaximepiton louismaximepiton force-pushed the main-lmp-pie-donut-charts branch from e6a43f7 to 3a74eeb Compare April 25, 2024 14:51
@louismaximepiton louismaximepiton marked this pull request as draft May 28, 2024 10:19
@louismaximepiton louismaximepiton force-pushed the main-lmp-pie-donut-charts branch from c43737c to 780f2cb Compare June 5, 2024 14:35
@louismaximepiton louismaximepiton marked this pull request as ready for review June 5, 2024 14:44
@louismaximepiton
Copy link
Member Author

Removed the work you did @jacques-lebourgeois since it'll be embedded in #242 and following PRs. I think it's ready for review.

Note: I've added the Orange legend look for now, but it'll have to be removed if the #242 is merged before or if the #242 is merged after, think about reverting 780f2cb

@jacques-lebourgeois
Copy link
Member

I just push a commit for suggestion if you agree

@louismaximepiton louismaximepiton force-pushed the main-lmp-pie-donut-charts branch 2 times, most recently from a126321 to fdd61ec Compare June 28, 2024 07:51
@Orange-OpenSource Orange-OpenSource deleted a comment from nbossard Jul 1, 2024
Copy link
Contributor

@julien-deramond julien-deramond left a comment

Choose a reason for hiding this comment

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

Good job with this addition @louismaximepiton
Few tiny things to change before accepting.
Please check #300 to see if there are other changes to do here while rebasing the branch ;)

docs/examples/pie-chart.html Outdated Show resolved Hide resolved
docs/examples/donut-chart.html Show resolved Hide resolved
@louismaximepiton louismaximepiton requested review from julien-deramond and removed request for julien-deramond July 8, 2024 14:12
@julien-deramond julien-deramond self-requested a review July 9, 2024 05:23
Copy link
Contributor

@julien-deramond julien-deramond left a comment

Choose a reason for hiding this comment

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

LGTM! Let's wait for the light design review, and then we can merge/release 🚀

@Franco-Riccitelli
Copy link
Member

Franco-Riccitelli commented Jul 19, 2024

Here are some design review comments for the Pie chart and Donut chart.

  • The use of the "Default" colours looks good to me. I can see these are based on the ODS Categorical colours for data displays.
  • The spacing between each of the segments in the charts is also very good.
  • The padding on the tooltips is quite large. For the Donut this can sometimes result in the tooltip overlapping the value in the centre of the Donut (when the chart is reduced in size). I suggest trying to reduce the padding in the tooltip by about half and also reducing the vertical space between the text labels in the tooltip by about a half.
  • When the charts are reduced in size, I also think the font size can be reduced for the legend and the text in the tooltip. I suggest trying 14px font size.
  • The legend should align with the title and subtitle at the top. At the moment, the legend is positioned slightly to the right.
  • The wrapping of the legend works well.

@julien-deramond
Copy link
Contributor

Thanks for the design review @Franco-Riccitelli 🙏

The padding on the tooltips is quite large.
I suggest trying to reduce the padding in the tooltip by about half and also reducing the vertical space between the text labels in the tooltip by about a half.

The padding on the tooltips follows the standard from ODS. We did not initially notice the different specification hidden at https://design.orange.com/0c1af118d/p/714762-legends/t/page-714762-48288420-5. Should we adhere to this specification instead for all charts? If so, this adjustment can be addressed in a separate PR, as it impacts the global rendering for all charts.

When the charts are reduced in size, I also think the font size can be reduced for the legend and the text in the tooltip. I suggest trying 14px font size.

We can implement this change in separate PRs since it affects the global rendering for all charts:

  • One PR for adjusting the label font size to 14px at small breakpoints
  • Another PR (linked to the previous feedback) for tooltip rendering adjustments

However, we need to determine the breakpoint at which the font size changes to 14px. Should this adjustment occur from tablet rendering to mobile (medium)?

The legend should align with the title and subtitle at the top. At the moment, the legend is positioned slightly to the right.

According to the ODS Design guidelines, the legend should be vertical and positioned on the right-hand side of the donut/pie chart. This change is not included in the current PR because it depends on the merge of another PR (PR #244). You can preview the updates in this deployment.
Do you confirm this must be positioned on the right-hand side of the donut/pie chart?

@Franco-Riccitelli
Copy link
Member

In response to the questions to my review of the Donut and Pie charts:

The padding for the tooltip should be reduced for all chart usage. I understand this will be a separate PR as this impacts the global rendering, which totally makes sense.

Font size change: I think the 14px font change should occur from the tablet to mobile breakpoints.

Legend positioning: The legend positioning at the bottom of the Pie and Donut chart works well and should remain at the bottom. The only slight adjustment is the alignment of the Legend to the Title and Subtitle (see attached example). Regarding the guidelines, I will update the Data Display Guidelines to show that the legend can be positioned at the bottom of Donut and Pie charts.
Screenshot 2024-07-22 at 10 30 38 AM

@julien-deramond
Copy link
Contributor

Thanks a lot for your feedback, @Franco-Riccitelli.
We've addressed the specific points related to the donut/pie charts in this PR. The more general feedback will be managed separately. We plan to create dedicated issues for those and implement the modifications through separate PRs.

@louismaximepiton louismaximepiton force-pushed the main-lmp-pie-donut-charts branch from bc5efab to f258adc Compare July 26, 2024 06:32
@louismaximepiton louismaximepiton force-pushed the main-lmp-pie-donut-charts branch from f258adc to 6ee3008 Compare July 26, 2024 06:37
@julien-deramond julien-deramond merged commit c55e002 into Orange-OpenSource:main Jul 26, 2024
5 checks passed
@nbossard
Copy link
Contributor

👏 good news

@louismaximepiton louismaximepiton deleted the main-lmp-pie-donut-charts branch July 26, 2024 08:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants