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

refactor: Replace usages of reactable in TimeTable #11239

Merged
merged 3 commits into from
Oct 26, 2020

Conversation

kgabryje
Copy link
Member

@kgabryje kgabryje commented Oct 12, 2020

SUMMARY

This PR refactors TimeTable to use TableView, based on react-table library. The goal was to get rid of reactable.
This PR replaces #11046, which was reverted due to the issue #11142.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

  • Has associated issue:
  • Changes UI
  • Requires DB Migration.
  • Confirm DB Migration upgrade and downgrade tested.
  • Introduces new feature or API
  • Removes existing feature or API

Sorry, something went wrong.

@kgabryje kgabryje marked this pull request as draft October 12, 2020 15:23
@kgabryje kgabryje changed the title refactor: Replace usages of reactable in TimeTable refactor: Replace usages of reactable in TimeTable (depends on #11217) Oct 12, 2020
@kgabryje
Copy link
Member Author

@michellethomas Can you please verify if the issue you described in #11142 doesn't occur on this branch?

@codecov-io
Copy link

codecov-io commented Oct 12, 2020

Codecov Report

Merging #11239 into master will decrease coverage by 3.95%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #11239      +/-   ##
==========================================
- Coverage   65.79%   61.83%   -3.96%     
==========================================
  Files         838      841       +3     
  Lines       39903    40159     +256     
  Branches     3662     3684      +22     
==========================================
- Hits        26253    24832    -1421     
- Misses      13549    15146    +1597     
- Partials      101      181      +80     
Flag Coverage Δ
#cypress ?
#javascript 62.93% <0.00%> (+0.25%) ⬆️
#python 61.17% <ø> (+0.23%) ⬆️

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

Impacted Files Coverage Δ
...rontend/src/visualizations/TimeTable/TimeTable.jsx 0.00% <0.00%> (ø)
superset-frontend/src/SqlLab/App.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/explore/App.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/dashboard/App.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/explore/index.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/dashboard/index.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/setup/setupColors.js 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/chart/ChartContainer.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/setup/setupFormatters.js 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/explore/reducers/index.js 0.00% <0.00%> (-100.00%) ⬇️
... and 191 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 5a3d23d...6080950. Read the comment docs.

@michellethomas
Copy link
Contributor

Hi @kgabryje it may take me a bit to set up my dev environment to be able to test and I don't want to block you (I'm working on a different project at the moment). Were you unable to reproduce the issue? If you add a time series table chart to a dashboard and add a filter that applies to the chart, and then you set/change the filter value, the chart often won't update the data in the chart (you can confirm this by going to explore to see the data with the filter).

If you aren't able to reproduce the issue, I can set up my environment and test in a few days.

@kgabryje kgabryje force-pushed the react-table/timetable branch from 940891e to 29e4161 Compare October 15, 2020 12:39
@kgabryje kgabryje changed the title refactor: Replace usages of reactable in TimeTable (depends on #11217) refactor: Replace usages of reactable in TimeTable Oct 15, 2020
@kgabryje kgabryje marked this pull request as ready for review October 15, 2020 12:40
@kgabryje kgabryje closed this Oct 15, 2020
@kgabryje kgabryje reopened this Oct 15, 2020
@kgabryje kgabryje closed this Oct 15, 2020
@kgabryje kgabryje reopened this Oct 15, 2020
@mistercrunch
Copy link
Member

I was able to pull the branch and confirm that the chart updates properly when setting dashboard-level filters.

Screenshot for reference:
Screen Shot 2020-10-18 at 10 41 52 PM

@mistercrunch
Copy link
Member

@michellethomas let us know if this small test is enough validation, or whether you still want to run more test in your environment.

@kgabryje
Copy link
Member Author

@mistercrunch Thanks for validating that!

@michellethomas
Copy link
Contributor

@mistercrunch just to double check you set a filter, removed it, and tried to set another filter and saw the data change each time? Do we know what the issue was with the previous change that caused this issue? If we know what led to the issue and that it was resolved, a simple test should be fine. I'll just do a double check in our weekly release whenever this gets in just to be sure.

class TimeTable extends React.PureComponent {
memoizedColumns = memoize(columnConfigs => [
Copy link
Member

Choose a reason for hiding this comment

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

While this memoized function only takes one parameter, and thus is not subject to that particular pitfall of lodash/memoize, it might be worth using memoize-one instead, or refactoring to a function component and using useMemo for safety.

})),
]);

memoizedRows = memoize((data, rows, columnConfigs) => {
Copy link
Member

Choose a reason for hiding this comment

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

oh... this one is going to have issues, from the look of it... we should definitely not be using lodash/memoize for this.

Copy link
Member

@rusackas rusackas left a comment

Choose a reason for hiding this comment

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

Let's replace lodash/memoize here, as I strongly suspect that'll cause issues.

@kgabryje
Copy link
Member Author

@rusackas Can you take a look? I've done the changes you requested

@rusackas
Copy link
Member

@kgabryje I'm still seeing lodash/memoize in use here in a couple places, one of which looks problematic. I'm hoping to add an eslint rule preventing use of this package soon, but let's not add new instances for now.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@kgabryje kgabryje force-pushed the react-table/timetable branch from 29e4161 to 6080950 Compare October 23, 2020 08:15
@kgabryje
Copy link
Member Author

@rusackas 🤦‍♂️ 🤦‍♂️ Sorry about that, I was sure that I pushed these changes. Now it should be alright - I refactored the Timetable into functional component and used useMemo.

@kgabryje kgabryje closed this Oct 23, 2020
@kgabryje kgabryje reopened this Oct 23, 2020
Copy link
Member

@rusackas rusackas left a comment

Choose a reason for hiding this comment

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

LGTM!

@rusackas rusackas merged commit 6acae9a into apache:master Oct 26, 2020
auxten pushed a commit to auxten/incubator-superset that referenced this pull request Nov 20, 2020
* Refactor TimeTable to use react-table

* Fix import

* Refactor TimeTable into functional component
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.0.0 labels Mar 12, 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/L 🚢 1.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants