chore: refactor ChartHolder to typescript + tests #20910

diegomedina248 · 2022-07-29T03:49:55Z

SUMMARY

This PR refactors the ChartHolder into a functional component with typescript.
It also adds more test coverage.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

--

TESTING INSTRUCTIONS

--

ADDITIONAL INFORMATION

Has associated issue:
Required feature flags:
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 · 2022-07-29T04:39:11Z

Codecov Report

Merging #20910 (d63f0de) into master (756ed0e) will increase coverage by 0.05%.
The diff coverage is 91.75%.

@@            Coverage Diff             @@
##           master   #20910      +/-   ##
==========================================
+ Coverage   66.29%   66.34%   +0.05%     
==========================================
  Files        1773     1774       +1     
  Lines       67680    67686       +6     
  Branches     7214     7215       +1     
==========================================
+ Hits        44866    44907      +41     
+ Misses      20971    20946      -25     
+ Partials     1843     1833      -10

Flag	Coverage Δ
javascript	`52.21% <91.75%> (+0.10%)`	⬆️

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

Impacted Files	Coverage Δ
...tend/src/dashboard/components/AnchorLink/index.tsx	`93.75% <ø> (+12.50%)`	⬆️
...ashboard/components/gridComponents/ChartHolder.tsx	`88.88% <88.88%> (ø)`
...end/src/dashboard/components/dnd/DragDroppable.jsx	`76.92% <100.00%> (ø)`
...rc/dashboard/util/useFilterFocusHighlightStyles.ts	`100.00% <100.00%> (ø)`
...-frontend/src/dashboard/reducers/dashboardState.js	`76.59% <0.00%> (+2.12%)`	⬆️
...hboard/components/resizable/ResizableContainer.jsx	`61.70% <0.00%> (+4.25%)`	⬆️

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

yousoph · 2022-07-29T18:55:36Z

/testenv up

yousoph · 2022-07-29T18:57:15Z

🏷️ Preset-Patch

github-actions · 2022-07-29T18:57:25Z

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

stephenLYZ · 2022-08-01T13:00:23Z

superset-frontend/src/dashboard/components/gridComponents/ChartHolder.tsx

+  );
+
+  const infoFromPath = useMemo(
+    () => getChartAndLabelComponentIdFromPath(directPathToChild) as any,


Is it possible to not use any here?

We could, but we'd need to change the file that contains that function to typescript, and there's a couple of types around the filters that are not well defined.
Plus, changing that file should follow with tests as well, and I didn't want to make this PR bigger, since it can be addressed as a follow up

Antonio-RiveroMartnez · 2022-08-04T14:09:17Z

superset-frontend/src/dashboard/components/gridComponents/ChartHolder.tsx

+  const [currentDirectPathLastUpdated, setCurrentDirectPathLastUpdated] =
+    useState(0);
+
+  const directPathToChild = useMemo(


I might be mistaken, but since we are not computing an expensive function, just accessing a property inside the dashboardState object, could we get rid of useMemo? Or there is some performance implication if we remove it, that I'm not seeing. Same with directPathLastUpdated below.

These two are used in other hooks/callbacks below, so need to memoize those to avoid recomputing everything (even if it's just a string/etc)

EugeneTorap · 2022-08-16T20:40:55Z

@diegomedina248 Thank you for this PR! I have a SqlEditor refactoring into FC, can I share the code with you?
@michael-s-molina @kgabryje Can we merge the PR?

diegomedina248 · 2022-08-16T21:10:04Z

@diegomedina248 Thank you for this PR! I have a SqlEditor refactoring into FC, can I share the code with you? @michael-s-molina @kgabryje Can we merge the PR?

@EugeneTorap sure thing!

jinghua-qa · 2022-08-17T07:30:26Z

/testenv up

github-actions · 2022-08-17T07:37:38Z

@jinghua-qa Ephemeral environment spinning up at http://54.214.218.74:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

github-actions · 2022-08-17T07:37:43Z

@jinghua-qa Ephemeral environment spinning up at http://54.214.218.74:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

jinghua-qa · 2022-08-18T18:03:46Z

I found some issues in this PR:
1, if user click on the filter indicator when chart is in fullscreen view, user will bring out to the normal view and the 3 dot menu on charts are not open. I test on superset master it seems like on master click on the filter indicator in enlarge chart view did not bring user to fullscreen view.

2, for numerical filter, it looks like the filter bar is not shown correctly when user select filter range:

diegomedina248 · 2022-08-22T12:04:40Z

I found some issues in this PR: 1, if user click on the filter indicator when chart is in fullscreen view, user will bring out to the normal view and the 3 dot menu on charts are not open. I test on superset master it seems like on master click on the filter indicator in enlarge chart view did not bring user to fullscreen view.

Screen.Recording.2022-08-18.at.11.06.53.AM.mov
2, for numerical filter, it looks like the filter bar is not shown correctly when user select filter range:

mm, I actually couldn't reproduce those.
I did rebase from master, to have the branch up to date, and couldn't reproduce either that way.

Here's what I'm doing:

test.mov

jinghua-qa · 2022-08-22T17:34:55Z

/testenv up

github-actions · 2022-08-22T18:05:17Z

@jinghua-qa Ephemeral environment spinning up at http://34.221.79.198:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

jinghua-qa · 2022-08-23T07:04:35Z

Test again in the new ephemeral env and verified the reported issue is fixed. Regression test for dashboard LGTM

EugeneTorap · 2022-08-29T14:05:12Z

@michael-s-molina @kgabryje @geido Can you review the PR and then merge it?

kgabryje

LGTM!
@diegomedina248 A suggestion/tip for future refactors like this one - if you use git mv to change filename, git diff will recognise the file as modified instead of 1 file deleted and 1 file created (like ChartHolder.jsx -> ChartHolder.tsx), which makes reviewing easier

github-actions · 2022-09-05T09:44:14Z

Ephemeral environment shutdown and build artifacts deleted.

pull-request-size bot added the size/XXL label Jul 29, 2022

yousoph added the need:qa-review Requires QA review label Jul 29, 2022

stephenLYZ reviewed Aug 1, 2022

View reviewed changes

rusackas requested a review from stephenLYZ August 3, 2022 22:24

Antonio-RiveroMartnez reviewed Aug 4, 2022

View reviewed changes

rusackas requested review from kgabryje, michael-s-molina and etr2460 August 16, 2022 04:49

chore: refactor ChartHolder to typescript + tests

d63f0de

diegomedina248 force-pushed the chore/refactor-chart-holder-to-ts branch from efe387e to d63f0de Compare August 22, 2022 11:51

kgabryje approved these changes Sep 5, 2022

View reviewed changes

kgabryje merged commit b71182f into apache:master Sep 5, 2022

mistercrunch added the 🚢 2.1.3 label Feb 18, 2024

mistercrunch added the 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels label Mar 13, 2024

mistercrunch added 🚢 2.1.0 and removed 🚢 2.1.3 labels Mar 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: refactor ChartHolder to typescript + tests #20910

chore: refactor ChartHolder to typescript + tests #20910

diegomedina248 commented Jul 29, 2022

codecov bot commented Jul 29, 2022 •

edited

Loading

yousoph commented Jul 29, 2022

yousoph commented Jul 29, 2022

github-actions bot commented Jul 29, 2022

stephenLYZ Aug 1, 2022

diegomedina248 Aug 1, 2022

Antonio-RiveroMartnez Aug 4, 2022

diegomedina248 Aug 4, 2022

EugeneTorap commented Aug 16, 2022

diegomedina248 commented Aug 16, 2022

jinghua-qa commented Aug 17, 2022

github-actions bot commented Aug 17, 2022

github-actions bot commented Aug 17, 2022

jinghua-qa commented Aug 18, 2022 •

edited

Loading

diegomedina248 commented Aug 22, 2022

jinghua-qa commented Aug 22, 2022

github-actions bot commented Aug 22, 2022

jinghua-qa commented Aug 23, 2022

EugeneTorap commented Aug 29, 2022 •

edited

Loading

kgabryje left a comment •

edited

Loading

github-actions bot commented Sep 5, 2022

chore: refactor ChartHolder to typescript + tests #20910

chore: refactor ChartHolder to typescript + tests #20910

Conversation

diegomedina248 commented Jul 29, 2022

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Jul 29, 2022 • edited Loading

Codecov Report

yousoph commented Jul 29, 2022

yousoph commented Jul 29, 2022

github-actions bot commented Jul 29, 2022

stephenLYZ Aug 1, 2022

Choose a reason for hiding this comment

diegomedina248 Aug 1, 2022

Choose a reason for hiding this comment

Antonio-RiveroMartnez Aug 4, 2022

Choose a reason for hiding this comment

diegomedina248 Aug 4, 2022

Choose a reason for hiding this comment

EugeneTorap commented Aug 16, 2022

diegomedina248 commented Aug 16, 2022

jinghua-qa commented Aug 17, 2022

github-actions bot commented Aug 17, 2022

github-actions bot commented Aug 17, 2022

jinghua-qa commented Aug 18, 2022 • edited Loading

diegomedina248 commented Aug 22, 2022

jinghua-qa commented Aug 22, 2022

github-actions bot commented Aug 22, 2022

jinghua-qa commented Aug 23, 2022

EugeneTorap commented Aug 29, 2022 • edited Loading

kgabryje left a comment • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Sep 5, 2022

codecov bot commented Jul 29, 2022 •

edited

Loading

jinghua-qa commented Aug 18, 2022 •

edited

Loading

EugeneTorap commented Aug 29, 2022 •

edited

Loading

kgabryje left a comment •

edited

Loading