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(plugin-chart-echarts): [feature-parity] support extra control for the area chart V2 #16493

Merged
merged 11 commits into from
Jun 8, 2022

Conversation

stephenLYZ
Copy link
Member

@stephenLYZ stephenLYZ commented Aug 29, 2021

🏆 Enhancements

This PR implements Extra Control on the area chart V2, and the chart's update mechanism is using setControlValue hook. There are some differences or improvements:

  • In explore the stack control will be updated if Extra Control is updated. But for NVD3, updating extra control does not update the stack control.
  • Here only keep Stack and Expend two options (Stream Graph will come soon) and add a new None option, which can cancel the stack style.
  • Another UI difference is the extra control button which uses the same radio control as the control panel instead of NVD3 radio button.

explore

before

2022-05-08.8.31.21.mov

after

2022-05-08.8.06.14.mov

dashboard

before

2022-05-08.8.31.59.mov

after

2022-05-08.8.35.59.mov

TESTING INSTRUCTIONS

  1. add Exta Control to the time-series area chart
  2. save and go to dashboard
  3. test the control and see results

ADDITIONAL INFORMATION

@codecov
Copy link

codecov bot commented Aug 29, 2021

Codecov Report

Merging #16493 (0a86dbb) into master (0e38c68) will decrease coverage by 0.01%.
The diff coverage is 54.92%.

@@            Coverage Diff             @@
##           master   #16493      +/-   ##
==========================================
- Coverage   66.54%   66.52%   -0.02%     
==========================================
  Files        1726     1727       +1     
  Lines       64875    64918      +43     
  Branches     6830     6843      +13     
==========================================
+ Hits        43169    43188      +19     
- Misses      19975    19997      +22     
- Partials     1731     1733       +2     
Flag Coverage Δ
javascript 51.35% <54.92%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
.../shared-controls/components/RadioButtonControl.tsx 0.00% <ø> (ø)
...hart-echarts/src/MixedTimeseries/transformProps.ts 0.00% <ø> (ø)
.../plugin-chart-echarts/src/MixedTimeseries/types.ts 100.00% <ø> (ø)
...chart-echarts/src/Timeseries/Area/controlPanel.tsx 40.00% <ø> (ø)
...chart-echarts/src/Timeseries/EchartsTimeseries.tsx 0.00% <ø> (ø)
...ugins/plugin-chart-echarts/src/Timeseries/types.ts 100.00% <ø> (ø)
...gin-chart-echarts/src/components/ExtraControls.tsx 0.00% <0.00%> (ø)
...frontend/plugins/plugin-chart-echarts/src/types.ts 100.00% <ø> (ø)
...et-frontend/src/components/Chart/ChartRenderer.jsx 54.23% <ø> (ø)
.../src/dashboard/components/gridComponents/Chart.jsx 60.60% <ø> (ø)
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0e38c68...0a86dbb. Read the comment docs.

@stephenLYZ stephenLYZ changed the title feat(echarts): [dashboard] support extra control for the time-series area chart feat(dashboard): [[feature-parity]] support extra control for the time-series area chart Aug 30, 2021
@stephenLYZ stephenLYZ changed the title feat(dashboard): [[feature-parity]] support extra control for the time-series area chart feat(dashboard): [feature-parity] support extra control for the time-series area chart Aug 30, 2021
@stephenLYZ stephenLYZ closed this Aug 30, 2021
@stephenLYZ stephenLYZ reopened this Aug 30, 2021
@junlincc junlincc added the viz:charts:echarts Related to Echarts label Aug 31, 2021
@pull-request-size pull-request-size bot added size/L and removed size/M labels Dec 1, 2021
@stephenLYZ stephenLYZ changed the title feat(dashboard): [feature-parity] support extra control for the time-series area chart feat(plugin-chart-echarts): [feature-parity] support extra control for the time-series area chart Dec 1, 2021
@stale
Copy link

stale bot commented Apr 16, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. For admin, please label this issue .pinned to prevent stale bot from closing the issue.

@stale stale bot added the inactive Inactive for >= 30 days label Apr 16, 2022
@jhult
Copy link
Contributor

jhult commented Apr 20, 2022

Any news on getting this merged?

@stale stale bot removed the inactive Inactive for >= 30 days label Apr 20, 2022
@stephenLYZ
Copy link
Member Author

@jhult I will finish this PR next week.

@stephenLYZ stephenLYZ changed the title feat(plugin-chart-echarts): [feature-parity] support extra control for the time-series area chart feat(plugin-chart-echarts): [feature-parity] support extra control for the area chart V2 May 8, 2022
@stephenLYZ
Copy link
Member Author

/testenv up FEATURE_GENERIC_CHART_AXES=true

@github-actions
Copy link
Contributor

@stephenLYZ Ephemeral environment spinning up at http://34.208.141.177:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

@jinghua-qa
Copy link
Member

LGTM!

Copy link
Member

@zhaoyongjie zhaoyongjie left a comment

Choose a reason for hiding this comment

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

Thanks for the awesome new feature! left some unblocked comments. If have time to add more unit-test in the separate PR.

@@ -191,7 +191,7 @@ export default function transformProps(
areaOpacity: opacity,
seriesType,
showValue,
stack,
stack: Boolean(stack),
Copy link
Member

Choose a reason for hiding this comment

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

no blocking nit

Suggested change
stack: Boolean(stack),
!!stack

setControlValue?: HandlerFunction;
}) {
const { stack, area } = formData;
const [extraValue, setExtraValue] = useState<JsonValue | undefined>(
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const [extraValue, setExtraValue] = useState<JsonValue | undefined>(
if (area === undefined) {
return []
}
const [extraValue, setExtraValue] = useState<JsonValue | undefined>(

@stephenLYZ stephenLYZ merged commit eab0009 into apache:master Jun 8, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Jun 8, 2022

Ephemeral environment shutdown and build artifacts deleted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/L viz:charts:echarts Related to Echarts 🚢 2.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants