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

feat: enable legend tab for column and bar (DHIS2-147) #1355

Merged
merged 7 commits into from
Oct 21, 2020

Conversation

martinkrulltott
Copy link
Contributor

@martinkrulltott martinkrulltott commented Oct 19, 2020

Implements DHIS2-147

Requires dhis2/analytics#650 and dhis2/analytics#656


Key features

  1. Enable the Legend tab for Column and Bar
  2. Refactor config to use props from Analytics instead of hardcoding them
  3. Fetch LegendSet names from the API

Description

Started out as a tiny change to enable the Legend tab for Column and Bar, the rest of the legend implementation can be found in the Analytics PR.

To prevent the config from mismatching with the Highcharts output, I refactored the config to use centralised props from Analytics instead of using hardcoded values. Meaning that the config will need minimal maintenance to work with future vis types.

LegendSet names are now fetched from the API, which are passed to Analytics to be used in the hover tooltip in Highcharts.


Screenshots

Legend tab enabled for Column
image

Legend tab enabled for Bar
image

VIS_TYPE_SINGLE_VALUE,
isStacked as getIsStacked,
Copy link
Member

Choose a reason for hiding this comment

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

Why the rename?
Perhaps it should be called isStackedType for consistency with the others?

Copy link
Contributor Author

@martinkrulltott martinkrulltott Oct 20, 2020

Choose a reason for hiding this comment

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

Because I wanted to pass isStacked to defaultProps without renaming the prop and obviously I can't have const isStacked = isStacked(type) 🙂 alternatively I could keep the original name of the Analytics prop and use a different name for my const, but I thought isStacked: someOtherName would be less clean the current way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The exported prop from Analytics could be renamed on the Analytics side but that would cause a breaking change that I don't think is worth it.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I thought about the breaking change.
Let's keep it as it is, or use isStacked as isStackedType 😆

@martinkrulltott martinkrulltott merged commit 04bb5a6 into master Oct 21, 2020
@martinkrulltott martinkrulltott deleted the feat/DHIS2-147-charts-legend-set branch October 21, 2020 13:09
dhis2-bot added a commit that referenced this pull request Oct 21, 2020
# [35.13.0](v35.12.34...v35.13.0) (2020-10-21)

### Features

* enable legend tab for column and bar (DHIS2-147) ([#1355](#1355)) ([04bb5a6](04bb5a6))
@dhis2-bot
Copy link
Contributor

🎉 This PR is included in version 35.13.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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.

4 participants