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

Adding a big number total viz type that is not a timeseries metric #212

Merged
merged 6 commits into from
Apr 7, 2016

Conversation

michellethomas
Copy link
Contributor

@williaster @mistercrunch

I'm adding a new big number total viz type that uses the same d3 as the big number but it is not a timeseries metric, so it will aggregate over whatever time period is given (this will allow us to do WAUs). I've also added functionality to add descriptive text below the number. I originally tried to adjust the text size based on the length but couldn't get it to work because of the loading order of the viz.

@landscape-bot
Copy link

Code Health
Code quality remained the same when pulling c239f81 on airbnb:big_number_total into d885dd3 on airbnb:master.

@landscape-bot
Copy link

Code Health
Code quality remained the same when pulling 2086607 on airbnb:big_number_total into d885dd3 on airbnb:master.

@@ -1,4 +1,4 @@
.big_number g.axis text {
.big_number g.axis text, .big_number_total g.axis text {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit, but style-wise (https://github.com/airbnb/css#formatting) each selector should have its own line when using multiple selectors in a rule declaration.

@williaster
Copy link
Contributor

a couple of nit picky things about js linting, overall looks good though!

@landscape-bot
Copy link

Code Health
Code quality remained the same when pulling 6cfeb0f on airbnb:big_number_total into 9a5a27a on airbnb:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.3%) to 70.863% when pulling 6cfeb0f on big_number_total into 9a5a27a on master.

@mistercrunch
Copy link
Member

To both showcase this new viz and get unit tests coverage you can add an example slice in caravel/data/__init__.py

"""Put emphasis on a single metric with this big number viz"""

viz_type = "big_number_total"
verbose_name = "Big Number Total"
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this new viz should be just "Big Number" and the previous one should say "Big Number with Trendline" or something. The viz_type we cannot change because of backwards compatibility, but the verbose_name we can change all we want.

@mistercrunch
Copy link
Member

As a side note it would be nice to have a time expression (much like now) called latest or something like that, that would basically add a WHERE time_column = (SELECT max(time_column) FROM table).

That might be brutal on the database's depending on indexes / partitioning, but maybe there could also be a table level setting called latest_expression or something...

@landscape-bot
Copy link

Code Health
Code quality remained the same when pulling 0dd20b0 on airbnb:big_number_total into bcca840 on airbnb:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.3%) to 71.546% when pulling 0dd20b0 on big_number_total into bcca840 on master.

@landscape-bot
Copy link

Code Health
Code quality remained the same when pulling d42279a on airbnb:big_number_total into bcca840 on airbnb:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 72.041% when pulling d42279a on big_number_total into bcca840 on master.

@landscape-bot
Copy link

Code Health
Code quality remained the same when pulling df141cb on airbnb:big_number_total into bcca840 on airbnb:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 72.041% when pulling df141cb on big_number_total into bcca840 on master.

@landscape-bot
Copy link

Code Health
Code quality remained the same when pulling e0eb74f on airbnb:big_number_total into bcca840 on airbnb:master.

@michellethomas
Copy link
Contributor Author

@williaster Thanks for all the feedback. Added fixes. Let me know if there's anything that still doesn't look good

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 72.041% when pulling e0eb74f on big_number_total into bcca840 on master.

stroke-width: 1px;
stroke: grey;
}

.big_number .domain {
.big_number .domain,
.big_number_total .domain{
Copy link
Contributor

Choose a reason for hiding this comment

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

don't worry about changing it for this diff, but going forward I'm trying to get people to adopt dash case for css eg .big-number-total

BEM is also an interesting approach if you feel like reading :) https://css-tricks.com/bem-101/

@@ -558,7 +558,7 @@ class BigNumberViz(BaseViz):
"""Put emphasis on a single metric with this big number viz"""

viz_type = "big_number"
verbose_name = "Big Number"
verbose_name = "Big Number with Trendline"
Copy link
Contributor

Choose a reason for hiding this comment

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

nice.

@williaster
Copy link
Contributor

LGTM! nice improvement! :)

@michellethomas michellethomas merged commit 59169bf into master Apr 7, 2016
@michellethomas michellethomas deleted the big_number_total branch April 8, 2016 18:15
graceguo-supercat pushed a commit to graceguo-supercat/superset that referenced this pull request Oct 4, 2021
zhaoyongjie pushed a commit to zhaoyongjie/incubator-superset that referenced this pull request Nov 17, 2021
zhaoyongjie pushed a commit to zhaoyongjie/incubator-superset that referenced this pull request Nov 24, 2021
zhaoyongjie pushed a commit to zhaoyongjie/incubator-superset that referenced this pull request Nov 25, 2021
zhaoyongjie pushed a commit to zhaoyongjie/incubator-superset that referenced this pull request Nov 26, 2021
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.8.6 labels Feb 19, 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.8.6
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants