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

Closes #338, #228: Improvements to x-axis formatting #365

Closed
wants to merge 66 commits into from
Closed

Conversation

emtwo
Copy link

@emtwo emtwo commented Apr 11, 2018

No description provided.

Allen Short and others added 30 commits March 5, 2018 22:34
(incorporates: show data source type when no doc link present (re #46))
Still can not use unpublished queries on dashboards.
addresses issue Madalin found that allows a click off the dialog box to
close it without resetting the textbox
but don’t name a table abcdefghijklmnop.
I tried to make
`JSON.stringify(this.visualization.options.columnMapping)` a variable
to avoid repeating it, but if I make it a `let` the linter throws an
error and if I make it a `const` then it doesn’t change with the UI and
the logic doesn’t work. :(

updated based on PR comments
Makes the _v of the schema browser toggle into a configurable string
per data source. It is not required and if it is not filled in the
toggle will not appear in the schema browser.
@emtwo emtwo force-pushed the emtwo/xaxis branch 2 times, most recently from 6fe90ee to 2cda756 Compare April 11, 2018 20:40
@jezdez
Copy link

jezdez commented Apr 26, 2018

r+

@emtwo Would you mind opening a PR upstream with this?

@emtwo
Copy link
Author

emtwo commented Apr 27, 2018

@jezdez thanks. Do we want to merge this here and also open the PR upstream? Or just open it upstream and wait for it merge there first?

@arikfr
Copy link

arikfr commented Apr 29, 2018

I've recently found out that Plotly has an option to auto detect the axis type:

type ( enumerated : "-" | "linear" | "log" | "date" | "category" )
default: "-"
Sets the axis type. By default, plotly attempts to determined the axis type by looking into the data of the traces that referenced the axis in question.

https://plot.ly/javascript/reference/#layout-scene-xaxis-type

Have you tried this instead?

@emtwo
Copy link
Author

emtwo commented May 7, 2018

Thanks @arikfr! That seems to work and is a nicer and more concise alternative.

Here are a couple of queries I used to test:

For testing YYYYMMDD formatting and having it automatically be detected:

WITH sample as (
    select '20171001' as x, 'US' as type, 574047 as foo union
    select '20171002' as x, 'US' as type, 574047 as foo union
    select '20171003' as x, 'US' as type, 574047 as foo union
    select '20171004' as x, 'US' as type, 574047 as foo union
    select '20171005' as x, 'US' as type, 574047 as foo union
    select '20171006' as x, 'US' as type, 574047 as foo union
    select '20171007' as x, 'US' as type, 574047 as foo union
    select '20171008' as x, 'US' as type, 574047 as foo union
    select '20171009' as x, 'US' as type, 574047 as foo union
    select '20171010' as x, 'US' as type, 574047 as foo union
    select '20171011' as x, 'US' as type, 574047 as foo union
    select '20171012' as x, 'US' as type, 574047 as foo union
    select '20171013' as x, 'US' as type, 574047 as foo union
    select '20171014' as x, 'US' as type, 574047 as foo union
    select '20171015' as x, 'US' as type, 574047 as foo union
    select '20171016' as x, 'US' as type, 574047 as foo union
    select '20171017' as x, 'US' as type, 574047 as foo union
    select '20171018' as x, 'US' as type, 574047 as foo union
    select '20171019' as x, 'US' as type, 574047 as foo union
    select '20171020' as x, 'US' as type, 574047 as foo union
    select '20171021' as x, 'US' as type, 574047 as foo union
    select '20171022' as x, 'US' as type, 8215767 as foo union
    select '20171023' as x, 'US' as type, 574047 as foo union
    select '20171024' as x, 'US' as type, 574047 as foo union
    select '20171025' as x, 'US' as type, 574047 as foo union
    select '20171026' as x, 'US' as type, 574047 as foo union
    select '20171027' as x, 'US' as type, 574047 as foo union
    select '20171028' as x, 'US' as type, 8215767 as foo union
    select '20171029' as x, 'US' as type, 574047 as foo union
    select '20171030' as x, 'US' as type, 8215767 as foo)
select * from sample

For testing another (category) automatic x-axis type detection. Using a as x axis and b and/or c as y axis:

WITH sample AS
    (select 'a' as a, 121370 as b, 21.58 as c union
     select 'b' as a, 121170 as b, 11.53 as c union
     select 'c' as a, 121170 as b, 22.58 as c union
     select 'd' as a, 121170 as b, 41.58 as c)
select a, b, c from sample

@@ -306,7 +306,7 @@ export default function init(ngModule) {
sortX: true,
legend: { enabled: true },
yAxis: [{ type: 'linear' }, { type: 'linear', opposite: true }],
xAxis: { type: 'datetime', labels: { enabled: true } },
xAxis: { type: '-', labels: { enabled: true } },
Copy link

Choose a reason for hiding this comment

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

You need to add this option to the xAxisScales array (and probably change it to have labels for each one, because currently we use the value itself, which won't work for -).

Copy link

Choose a reason for hiding this comment

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

Okay that makes sense, @emtwo could take another look at this?

@arikfr
Copy link

arikfr commented May 8, 2018

👍
feel free to push this upstream as well :)

@jezdez
Copy link

jezdez commented May 9, 2018

@emtwo Let's merge this here first and then open an upstream PR at the same time as well. I'm not sure how quick we can expect merging upstream into our master.

@emtwo
Copy link
Author

emtwo commented May 28, 2018

Just to follow up on this, there is a PR upstream that is currently under review with some comments recently addressed: getredash#2542

I didn't want to merge this PR here if it was going to change during the review process upstream, so I've left it open.

@jezdez
Copy link

jezdez commented Jun 19, 2018

@emtwo The PR getredash#2542 has been merged upstream, is there work needed here to extend it in our fork with functionality that wasn't accepted?

@emtwo
Copy link
Author

emtwo commented Jun 20, 2018

@jezdez My suggested portion of the patch here: 7fb937d to resolve #338 was not accepted because the solution would do a regex match on large numbers, not just dates.

A couple of good suggestions are here: getredash#2542 (comment)

Though, in my opinion, it may be best/easiest to just stick with the first suggestion there, which is what we're already doing or have the jobs that generate/process the data making it into Athena to convert YYYYMMDD to YYYY-MM-DD.

I'm going to close this PR and #228 as it's being handled upstream.

@emtwo emtwo closed this Jun 20, 2018
@jezdez
Copy link

jezdez commented Jun 20, 2018

@emtwo Excellent, I fully agree. Thanks for your patience getting this upstreamed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants