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

Feature: Display the verbose name for metrics within Timeseries charts and legend. #3504

Merged
merged 1 commit into from
Sep 26, 2017

Conversation

fabianmenges
Copy link
Contributor

This solves the verbose name issues, like #3437, but only for nvd3Vis charts (line charts, bar charts, area charts, etc...)

The entire renaming logic was moved into JS and the presentation layer. This should address the issues that @mistercrunch brought up. I'm happy to close the other MR if we decide to go with this approach.

No need for new screenshots, they look the same.

@coveralls
Copy link

coveralls commented Sep 20, 2017

Coverage Status

Coverage increased (+0.03%) to 69.501% when pulling 5bfe926cd918e8b9dde0b79aa49a54a6cc8f405b on tc-dc:fmenges/verbose_names_timeseries into ae7f163 on apache:master.

@coveralls
Copy link

coveralls commented Sep 20, 2017

Coverage Status

Coverage increased (+0.03%) to 69.496% when pulling cd50d2d5428e671653e2a07d1b7bec98540fb19e on tc-dc:fmenges/verbose_names_timeseries into 1cf634a on apache:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 69.567% when pulling 4c72864 on tc-dc:fmenges/verbose_names_timeseries into 9af34ba on apache:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 69.567% when pulling 4c72864 on tc-dc:fmenges/verbose_names_timeseries into 9af34ba on apache:master.

@coveralls
Copy link

coveralls commented Sep 21, 2017

Coverage Status

Coverage increased (+0.03%) to 69.567% when pulling 4c72864 on tc-dc:fmenges/verbose_names_timeseries into 9af34ba on apache:master.

@mistercrunch mistercrunch merged commit 8efcaeb into apache:master Sep 26, 2017
series_title = name
else:
name = ["{}".format(s) for s in name]
if len(self.form_data.get('metrics')) > 1:
Copy link
Member

Choose a reason for hiding this comment

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

Users reported an issue related to removing this logic. In the case where there's a single metric and a one or many dimension, the metric name would not be reported prior to this change, which is the desired behavior. Where before using metric count with gender dimension would give ['female', 'male', 'other'] in both the legend and tooltip, now it says ['count, female', 'count, male', 'count, other']

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please don't report this to me if you don't want me to fix it. Just wasted 30min on context switching and pushing a branch only to find your fix...

@fabianmenges
Copy link
Contributor Author

Should be easy to fix, I'll try to push something later today...

timifasubaa pushed a commit to timifasubaa/incubator-superset that referenced this pull request Oct 3, 2017
michellethomas pushed a commit to michellethomas/panoramix that referenced this pull request May 24, 2018
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.20.1 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.20.1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants