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

243-enabled-different-kinds-of-legend #244

Merged
merged 16 commits into from
Sep 20, 2024

Conversation

jacques-lebourgeois
Copy link
Member

@jacques-lebourgeois jacques-lebourgeois commented May 28, 2024

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

Related issues

Closes #243

Closes #245 (via #246)

Description

Specific legend holders

Motivation & Context

Outsourcing captions should make it possible to

  • position legends vertically or
  • separate them into different containers

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-echarts
  • test/html
  • test/react
  • test/vue
  • test/examples/bar-line-chart
  • test/examples/single-line-chart
  • test/examples/timeseries-chart

@jacques-lebourgeois jacques-lebourgeois linked an issue May 28, 2024 that may be closed by this pull request
2 tasks
Copy link

netlify bot commented May 28, 2024

Deploy Preview for ods-charts ready!

Name Link
🔨 Latest commit 0d7b923
🔍 Latest deploy log https://app.netlify.com/sites/ods-charts/deploys/66e02daecf02ea0008ae6c19
😎 Deploy Preview https://deploy-preview-244--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-assigned this May 28, 2024
@jacques-lebourgeois jacques-lebourgeois force-pushed the 243-enabled-different-kinds-of-legend branch 2 times, most recently from 08876f1 to 57409d7 Compare May 29, 2024 08:05
@jacques-lebourgeois jacques-lebourgeois changed the base branch from main to 241-add-orange-design-system-lookfeel-to-default-apache-echarts-legends May 29, 2024 08:06
@jacques-lebourgeois jacques-lebourgeois force-pushed the 241-add-orange-design-system-lookfeel-to-default-apache-echarts-legends branch 2 times, most recently from 06953a1 to e394a05 Compare June 14, 2024 13:13
Base automatically changed from 241-add-orange-design-system-lookfeel-to-default-apache-echarts-legends to main June 14, 2024 13:16
@jacques-lebourgeois jacques-lebourgeois force-pushed the 243-enabled-different-kinds-of-legend branch from 57409d7 to 1a867d1 Compare June 14, 2024 13:36
@jacques-lebourgeois jacques-lebourgeois force-pushed the 243-enabled-different-kinds-of-legend branch from 1a867d1 to 98a2598 Compare June 21, 2024 15:32
Copy link
Member

@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.

Using this solution, should we handle the tooltip legend as well ? I don't know if this is feasible automatically and if it should be done in here but something like:
image

On the JS part, haven't checked in details, but it seems to work pretty well. Some small concerns (that might not be related to the PR):

  • If a series is empty, the legend appear on the bottom but not in the tooltip, are we fine with that even if it's not consistent?
  • If there is no defaultLegendHolder, some elements of the legend could be hidden, do you think we should handle that use case?

src/theme/legends/ods-chart-legends-definitions.ts Outdated Show resolved Hide resolved
docs/use_cases/legends-holders.html Outdated Show resolved Hide resolved
docs/use_cases/legends-holders.html Outdated Show resolved Hide resolved
docs/use_cases/legends-holders.html Outdated Show resolved Hide resolved
docs/use_cases/legends-holders.html Outdated Show resolved Hide resolved
docs/use_cases/legends-holders.html Outdated Show resolved Hide resolved
docs/use_cases/legends-holders.html Outdated Show resolved Hide resolved
src/theme/legends/ods-chart-legends.ts Outdated Show resolved Hide resolved
@jacques-lebourgeois
Copy link
Member Author

Using this solution, should we handle the tooltip legend as well ? I don't know if this is feasible automatically and if it should be done in here but something like ...

I hadn't thought of this use case. I think it's a good idea. But it seems rather complex to do simply in this Merge Request, don't you think? We can wait for a project need before implementing it, what do you think? Note that it's always possible for the project to fully customize the tooltip content.

@jacques-lebourgeois
Copy link
Member Author

If a series is empty, the legend appear on the bottom but not in the tooltip, are we fine with that even if it's not consistent?

Agree, but it is not specific to this PR : cf code of https://codepen.io/jacques-lebourgeois/pen/rNgzbXQ

it should be treated as a bug or a new feature to be able to configure legends with an option hide empty series legend

@jacques-lebourgeois
Copy link
Member Author

If there is no defaultLegendHolder, some elements of the legend could be hidden, do you think we should handle that use case?

Not sure because I wouldn't know which container to choose. What's more, it may be the developer's wish not to display legend for so-called “technical” series.

Co-authored-by: Louis-Maxime Piton <louismaxime.piton@orange.com>
Co-authored-by: Louis-Maxime Piton <louismaxime.piton@orange.com>
Co-authored-by: Louis-Maxime Piton <louismaxime.piton@orange.com>
@jacques-lebourgeois jacques-lebourgeois force-pushed the 243-enabled-different-kinds-of-legend branch from 946c124 to b5ab6a5 Compare September 2, 2024 12:43
docs/use_cases/legends-holders.html Outdated Show resolved Hide resolved
docs/use_cases/legends-holders.html Outdated Show resolved Hide resolved
* 245-add-vertical-legends-management-in-documentation-tool

* Make donut and pie charts work

* Fix formatting

* [FIX] we save some space for it even if it is not rendered

---------

Co-authored-by: Julien Déramond <julien.deramond@orange.com>
Co-authored-by: Louis-Maxime Piton <louismaxime.piton@orange.com>
Copy link
Member

@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.

LGTM 🚀

@julien-deramond
Copy link
Member

julien-deramond commented Sep 6, 2024

@jacques-lebourgeois In https://ods-charts.netlify.app/examples/bar-chart, in the "Vertical grouped bar chart", we had the possibility to switch between "ODS Charts legend" and "Apache ECharts legend". That's not the case anymore in this PR, and so it's not possible neither to change here the legend orientation. Is it voluntary?

…switch between "ODS Charts legend" and "Apache ECharts legend". => Add a missing css selector context
@jacques-lebourgeois
Copy link
Member Author

@jacques-lebourgeois In https://ods-charts.netlify.app/examples/bar-chart, in the "Vertical grouped bar chart", we had the possibility to switch between "ODS Charts legend" and "Apache ECharts legend". That's not the case anymore in this PR, and so it's not possible neither to change here the legend orientation. Is it voluntary?

You're right, thanks.

The selector of type legend renderer is now hidden when there is no legend.

But I forgot a context in my css selector and then hide this selector for all the charts.

Sorry, this is fixed I hope

Copy link
Member

@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.

Neat @jacques-lebourgeois! Good job.
I've made some minor modifications in f32553a, 83d25d5, 715b4e1, and cd8cc09.
I let you check that everything's OK, and then I can merge and release a new alpha version 🚀

@jacques-lebourgeois jacques-lebourgeois merged commit 7e5c9c7 into main Sep 20, 2024
5 checks passed
@jacques-lebourgeois jacques-lebourgeois deleted the 243-enabled-different-kinds-of-legend branch September 20, 2024 13:12
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.

Add vertical legends management in documentation tool Enabled different kinds of legend.
3 participants