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(cross-filters): using verbose map in applied cross-filters #23509

Merged

Conversation

Always-prog
Copy link
Contributor

@Always-prog Always-prog commented Mar 28, 2023

SUMMARY

Displaying human-readable column names in cross-filters by using verbose mapping from the chart's datasource.

BEFORE

Screenshot_46

AFTER

Screenshot_45

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags: DASHBOARD_CROSS_FILTERS
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@codecov
Copy link

codecov bot commented Mar 28, 2023

Codecov Report

Merging #23509 (5015e3c) into master (0cebe8b) will decrease coverage by 11.17%.
The diff coverage is 59.52%.

❗ Current head 5015e3c differs from pull request most recent head f8a1d6e. Consider uploading reports for the commit f8a1d6e to get more accurate results

@@             Coverage Diff             @@
##           master   #23509       +/-   ##
===========================================
- Coverage   67.65%   56.48%   -11.17%     
===========================================
  Files        1910     1914        +4     
  Lines       73745    73939      +194     
  Branches     7987     8024       +37     
===========================================
- Hits        49891    41766     -8125     
- Misses      21813    30128     +8315     
- Partials     2041     2045        +4     
Flag Coverage Δ
javascript 53.89% <54.94%> (+0.04%) ⬆️
mysql ?
postgres ?
sqlite ?

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

Impacted Files Coverage Δ
...t-ui-chart-controls/src/shared-controls/mixins.tsx 16.66% <ø> (ø)
.../packages/superset-ui-core/src/chart/types/Base.ts 100.00% <ø> (ø)
...s/superset-ui-core/src/components/SafeMarkdown.tsx 66.66% <ø> (ø)
...gins/legacy-plugin-chart-world-map/src/WorldMap.js 0.00% <0.00%> (ø)
...plugins/legacy-plugin-chart-world-map/src/index.js 66.66% <ø> (ø)
.../plugins/plugin-chart-echarts/src/BoxPlot/index.ts 50.00% <ø> (ø)
...d/plugins/plugin-chart-echarts/src/Funnel/index.ts 50.00% <ø> (ø)
...nd/plugins/plugin-chart-echarts/src/Gauge/index.ts 50.00% <ø> (ø)
...ns/plugin-chart-echarts/src/Graph/EchartsGraph.tsx 0.00% <0.00%> (ø)
...nd/plugins/plugin-chart-echarts/src/Graph/index.ts 50.00% <ø> (ø)
... and 57 more

... and 298 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@Always-prog
Copy link
Contributor Author

@villebro Hello! Can I get review of my PR?

@villebro villebro self-requested a review March 29, 2023 10:05
Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

LGTM with one stylistic nit and tested to work as expected. Great work btw!

@villebro villebro requested a review from kgabryje March 29, 2023 10:20
@kgabryje
Copy link
Member

/testenv up FEATURE_DASHBOARD_CROSS_FILTERS=true

@github-actions
Copy link
Contributor

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

@kgabryje
Copy link
Member

kgabryje commented Mar 29, 2023

Looks great, thanks for contribution!
1 nit - when we do const charts = useSelector(state => state.charts);, the component will rerender each time any chart changes (for example, when a dashboard with 10 charts loads, the charts change their chartStatus loading -> success -> rendered - that's 30 changes/rerenders already).
It looks like getVerboseMap actually only needs chart ids, not full chart objects, so could we do something like const chartIds = useSelector(state => Object.keys(state.charts)); instead?

We actually have many instances of unnecessarily wide selection of redux state throughout the app, hoping to clean it up some day 😄

@Always-prog
Copy link
Contributor Author

@kgabryje Thank you for review!
I'll optimize getVerboseMapsForCharts function today

@kgabryje
Copy link
Member

Another improvement that we could use to DRY things up is to transform getVerboseMapsForCharts into a react hook, so that we don't need to select datasources and charts in each component where we want to get the verbose maps. But that's optional 🙂

@Always-prog
Copy link
Contributor Author

@kgabryje Hello!
I updated GetChartsVerboseMaps function, it return charts verbose maps one time without rerender, and it doesn't need any params.
Can you please review code?
Thank you!

@kgabryje
Copy link
Member

@Always-prog Tested, works great! 1 last nit - the naming conventions for functions is to start them with lowercase letter + if it's a hook, start the name with use. Could you rename GetChartsVerboseMaps to useChartsVerboseMaps?

@Always-prog
Copy link
Contributor Author

@kgabryje Of course, renamed in the last commit.

@Always-prog
Copy link
Contributor Author

@kgabryje Hello!
I renamed the function, can you review my code please?

Copy link
Member

@kgabryje kgabryje left a comment

Choose a reason for hiding this comment

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

LGTM!

@kgabryje kgabryje merged commit bc2ec04 into apache:master Mar 31, 2023
@github-actions
Copy link
Contributor

Ephemeral environment shutdown and build artifacts deleted.

@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 3.0.0 labels Mar 13, 2024
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/M 🚢 3.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants