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

added a 'no_trend_line' option #5356

Merged
merged 2 commits into from
Jul 6, 2018
Merged

Conversation

xiaoyugit
Copy link
Contributor

screen shot 2018-07-06 at 20 27 33

@codecov-io
Copy link

Codecov Report

Merging #5356 into master will decrease coverage by <.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5356      +/-   ##
==========================================
- Coverage   61.33%   61.33%   -0.01%     
==========================================
  Files         369      369              
  Lines       23488    23489       +1     
  Branches     2713     2714       +1     
==========================================
  Hits        14407    14407              
- Misses       9069     9070       +1     
  Partials       12       12
Impacted Files Coverage Δ
superset/assets/src/explore/visTypes.js 55.55% <ø> (ø) ⬆️
superset/assets/src/explore/controls.jsx 47.32% <ø> (ø) ⬆️
superset/assets/src/visualizations/big_number.js 8.24% <0%> (-0.09%) ⬇️

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 252cba2...6feec49. Read the comment docs.

@mistercrunch mistercrunch merged commit 7158fb1 into apache:master Jul 6, 2018
@williaster
Copy link
Contributor

@xiaohanyu @mistercrunch it would have been great to clean up the code and consolidate vis types, deprecating the "big number" without trend line, in this PR. Now we have two vis types that essentially do the same thing (one with a superset of functionality over the other).

@mistercrunch
Copy link
Member

@williaster this one will take the latest available dot in the time series where "Big Number" would disregard the time granularity.

Personally I'm ok with a revert on this if you prefer. I know you had touched big_number.js in the past and there's an open PR on this. Happy to defer to that.

I'll make sure to keep PRs open longer before merging them to allow for more input.

@xiaohanyu
Copy link
Contributor

@williaster I'm sorry but you @ the wrong person, LOL~

@xiaoyugit
Copy link
Contributor Author

@xiaohanyu LOL~ guess I am the wrong person.
@williaster @mistercrunch given different ways of handling time granularity, I don't think these two plots are redundant on functionality. Personally I would love to keep a number plot with Month-over-Month comparison but without the trend line (for tidiness).

@williaster
Copy link
Contributor

@xiaohanyu sorry about that! 🙉

@mistercrunch @xiaoyugit thanks for the thoughts. I think these two are still really redundant and the granularity difference could be handled with more config options. Will keep the two for now, though, since that's more work / would need a migration.

timifasubaa pushed a commit to airbnb/superset-fork that referenced this pull request Jul 25, 2018
* added a 'no_trend_line' option

* added missing comma
kristw pushed a commit to kristw/incubator-superset that referenced this pull request Aug 9, 2018
* added a 'no_trend_line' option

* added missing comma

(cherry picked from commit 7158fb1)
wenchma pushed a commit to wenchma/incubator-superset that referenced this pull request Nov 16, 2018
* added a 'no_trend_line' option

* added missing comma
@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.

None yet

5 participants