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

[SIP-5] Refactor table #5707

Merged
merged 17 commits into from
Aug 31, 2018
Merged

[SIP-5] Refactor table #5707

merged 17 commits into from
Aug 31, 2018

Conversation

kristw
Copy link
Contributor

@kristw kristw commented Aug 23, 2018

  • Extract slice and formData according to [SIP-5]

@williaster @conglei @graceguo-supercat

@codecov-io
Copy link

codecov-io commented Aug 23, 2018

Codecov Report

Merging #5707 into master will increase coverage by 0.03%.
The diff coverage is 90.14%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5707      +/-   ##
==========================================
+ Coverage   63.75%   63.78%   +0.03%     
==========================================
  Files         364      364              
  Lines       23104    23110       +6     
  Branches     2579     2587       +8     
==========================================
+ Hits        14729    14740      +11     
+ Misses       8360     8355       -5     
  Partials       15       15
Impacted Files Coverage Δ
superset/assets/src/chart/Chart.jsx 58.4% <ø> (+0.7%) ⬆️
superset/assets/src/visualizations/table.js 85.82% <90.14%> (+3.06%) ⬆️

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 f72cdc3...1d034cf. Read the comment docs.

filters,
includeSearch,
metrics: rawMetrics,
onAddFilter = NOOP,
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need a No Op on these prop functions?

Copy link
Contributor Author

@kristw kristw Aug 24, 2018

Choose a reason for hiding this comment

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

Set NOOP as default value to avoid checking if onAddFilter exists every time before calling it.

@kristw kristw changed the title Refactor table [SIP-5] Refactor table Aug 24, 2018
Copy link
Contributor

@williaster williaster left a comment

Choose a reason for hiding this comment

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

looks great to me! sorry for the cryptic test bug :(

I had a couple comments about some css that could possibly be removed. but I wouldn't block on that.

import tableVis from '../../../src/visualizations/table';

// Fix `Option is not defined`
// https://stackoverflow.com/questions/39501589/jsdom-option-is-not-defined-when-running-my-mocha-test
global.Option = window.Option;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if it'd be useful to put this into the spec/helpers/browser.js file, that's where other things like this currently are. but once we move to packages, this type of config should be specific to each package.

// Each object is { field1: value1, field2: value2 }
data: PropTypes.arrayOf(PropTypes.object),
height: PropTypes.number,
alignPn: PropTypes.bool,
Copy link
Contributor

Choose a reason for hiding this comment

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

"Pn" means positive/negative? could we update that variable name to be more descriptive to indicate that?

@@ -1,5 +1,5 @@
.slice-grid .widget.table .slice_container {
Copy link
Contributor

Choose a reason for hiding this comment

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

possibly good news: anything with .widget I think can be removed. this is what grid cells on the old dashboard used to be called.

}
datatable.draw();
container.parents('.widget').find('.tooltip').remove();
$container.parents('.widget').find('.tooltip').remove();
Copy link
Contributor

Choose a reason for hiding this comment

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

I would guess that this tried to remove tooltips from other dashboard charts 🤔

pageLength: pageLength && parseInt(pageLength, 10),
percentMetrics,
// Aug 22, 2018
// Perhaps this `tableFilter` field can be removed as there is
Copy link
Contributor

Choose a reason for hiding this comment

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

that's interesting. I think this table used to be able to function as a filter on dashboards but perhaps that broke at some point. @mistercrunch any thoughts?

@kristw
Copy link
Contributor Author

kristw commented Aug 31, 2018

Addressed comments, except the tableFilter one.

@williaster williaster merged commit 8a4b1b7 into apache:master Aug 31, 2018
@kristw kristw deleted the kristw-table branch September 5, 2018 18:52
kristw added a commit to kristw/incubator-superset that referenced this pull request Sep 11, 2018
* update indent

* extract formData and data.

* take filter

* remove dependencies

* remove removeFilter

* add comment

* remove columnFormats and verboseMap from props. clarify a few more props

* fix linting issue

* minor syntax

* syntax fix

* Move check to adaptor

* update unit test

* remove code related to .widget

* rename variables for clarity

* move Option fix to browser.js

(cherry picked from commit 8a4b1b7)
betodealmeida pushed a commit to lyft/incubator-superset that referenced this pull request Oct 12, 2018
* update indent

* extract formData and data.

* take filter

* remove dependencies

* remove removeFilter

* add comment

* remove columnFormats and verboseMap from props. clarify a few more props

* fix linting issue

* minor syntax

* syntax fix

* Move check to adaptor

* update unit test

* remove code related to .widget

* rename variables for clarity

* move Option fix to browser.js
wenchma pushed a commit to wenchma/incubator-superset that referenced this pull request Nov 16, 2018
* update indent

* extract formData and data.

* take filter

* remove dependencies

* remove removeFilter

* add comment

* remove columnFormats and verboseMap from props. clarify a few more props

* fix linting issue

* minor syntax

* syntax fix

* Move check to adaptor

* update unit test

* remove code related to .widget

* rename variables for clarity

* move Option fix to browser.js
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.28.0 labels Feb 27, 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 🚢 0.28.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants