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.

Sign up for GitHub

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

Jump to bottom

bugfix: improve 'Time Table' #6959

Merged
merged 5 commits into from
Apr 12, 2019
Merged

bugfix: improve 'Time Table' #6959

merged 5 commits into from
Apr 12, 2019

Conversation

mistercrunch
Copy link
Member

closes #6948

@vylc was able to reproduce and also hit unrelated issues around Time Table. Started work here but reached out for help on Slack.

screen shot 2019-02-27 at 11 29 44 pm

@codecov-io
Copy link

codecov-io commented Mar 13, 2019 •
edited
Loading

Codecov Report

Merging #6959 into master will decrease coverage by <.01%.
The diff coverage is 40%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6959      +/-   ##
==========================================
- Coverage   64.93%   64.93%   -0.01%     
==========================================
  Files         424      424              
  Lines       20592    20597       +5     
  Branches     2280     2281       +1     
==========================================
+ Hits        13372    13374       +2     
- Misses       7097     7100       +3     
  Partials      123      123
Impacted Files Coverage Δ
...re/components/controls/TimeSeriesColumnControl.jsx 65.9% <ø> (ø) ⬆️
superset/assets/src/explore/controls.jsx 42.4% <ø> (ø) ⬆️
.../explore/components/controls/CollectionControl.jsx 20.58% <0%> (-2%) ⬇️
superset/assets/src/utils/getClientErrorObject.js 84.21% <100%> (+1.85%) ⬆️

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 763db8f...cfef272. Read the comment docs.

@vylc vylc added the lyft Related to Lyft label Mar 28, 2019
@michellethomas
Copy link
Contributor

@mistercrunch have you dug into this anymore? We are running into the error Converting circular structure to JSON in 0.31 with adding anything to time table.

When I remove this line I don't get the error. It looks like all props are getting added to each object in value, which creates a circular reference where each object in value contains value (the list of time series column params). Do you remember why you were adding props to the control here?

@mistercrunch
Copy link
Member Author

@michellethomas this is ready for review!

@mistercrunch mistercrunch changed the title [WiP] debugging and improving 'Time Table' bugfix: improve 'Time Table' Apr 11, 2019
michellethomas
michellethomas approved these changes Apr 11, 2019
View reviewed changes
Copy link
Contributor

@michellethomas michellethomas 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 fixing this, one question about the console logs but lgtm otherwise

@mistercrunch mistercrunch merged commit 9b4f5ad into apache:master Apr 12, 2019
@mistercrunch mistercrunch deleted the debug_time_table branch April 12, 2019 06:36
@khtruong khtruong mentioned this pull request Apr 15, 2019
Merged
12 tasks
@michellethomas michellethomas added v0.31 !deprecated-label:bug Deprecated label - Use #bug instead labels Apr 15, 2019
michellethomas pushed a commit that referenced this pull request Apr 15, 2019
@mistercrunch @michellethomas
bugfix: improve 'Time Table' (#6959)
24a595f 
* [WiP] debugging and improving 'Time Table'

closes #6948

* Lint

* Remove passing down props from CollectionControl

* Declarative passthrough of props

* remove console.error

(cherry picked from commit 9b4f5ad)
khtruong added a commit to lyft/incubator-superset that referenced this pull request Apr 15, 2019
@khtruong
Revert "bugfix: improve 'Time Table' (apache#6959)"
9154220 
This reverts commit 9b4f5ad.
betodealmeida pushed a commit that referenced this pull request Apr 16, 2019
@khtruong @betodealmeida
Revert "bugfix: improve 'Time Table' (#6959)" (#7304)
154f6ab 
This reverts commit 9b4f5ad.
michellethomas pushed a commit that referenced this pull request Apr 17, 2019
@khtruong @michellethomas
Revert "bugfix: improve 'Time Table' (#6959)" (#7304)
e8aa081 
This reverts commit 9b4f5ad.

(cherry picked from commit 154f6ab)
michellethomas pushed a commit that referenced this pull request May 15, 2019
@khtruong @michellethomas
Revert "bugfix: improve 'Time Table' (#6959)" (#7304)
abd4b26 
This reverts commit 9b4f5ad.

(cherry picked from commit 154f6ab)
@mistercrunch mistercrunch added the 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels label Feb 28, 2024
cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024
@khtruong @michellethomas
Revert "bugfix: improve 'Time Table' (apache#6959)" (apache#7304)
93cd794 
This reverts commit 9b4f5ad.

(cherry picked from commit 154f6ab)
cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024
@khtruong @michellethomas
Revert "bugfix: improve 'Time Table' (apache#6959)" (apache#7304)
642ace6 
This reverts commit 4d36a4b.

(cherry picked from commit 824d578)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xtinec xtinec xtinec left review comments

@michellethomas michellethomas michellethomas approved these changes

Assignees
No one assigned
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels !deprecated-label:bug Deprecated label - Use #bug instead lyft Related to Lyft v0.31 🚢 0.34.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[time series table] On viz type change default to adding a Time Series column entry
5 participants
@mistercrunch @codecov-io @michellethomas @xtinec @vylc