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: date range selector support for charts #594

Merged
merged 18 commits into from Nov 3, 2015

Conversation

tjwudi
Copy link
Contributor

@tjwudi tjwudi commented Oct 8, 2015

screen shot 2015-10-08 at 2 57 34 pm

Date range selector allows you to zoom in & zoom out by date range. Although, we can use HighStock to achieve this, it is quite a big overhead, both to performance and engineering time. It is quite simple though.

Replace the whole dateRange object in scope instead of changing min and max properties one-by-one. Given how angular `$watch` works with Moment.js object, I wrote some comment to clarify the right way to update dateRange.
Signed-off-by: John Wu <webmaster@leapoahead.com>
Signed-off-by: John Wu <webmaster@leapoahead.com>
Signed-off-by: John Wu <webmaster@leapoahead.com>
…nge selector

Signed-off-by: John Wu <webmaster@leapoahead.com>
Signed-off-by: John Wu <webmaster@leapoahead.com>
Signed-off-by: John Wu <webmaster@leapoahead.com>
Signed-off-by: John Wu <webmaster@leapoahead.com>
`seriesCollection` -> `allSeries` (shorter)
`s` -> `series`

Signed-off-by: John Wu <webmaster@leapoahead.com>
@landscape-bot
Copy link

Code Health
Repository health increased by 0.15% when pulling cc91981 on tjwudi:diwu/feature/date-range-selector into c01d88c on EverythingMe:master.

/**
* Update date range by finding date extremes
*
* ISSUE: chart.getExtreme() does not support getting Moment object out of box
Copy link
Member

Choose a reason for hiding this comment

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

If chart.getExtreme works right but the only problem is that it returns non Moment objects, we can convert them. Might be easier than searching ourselves.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it simply returns something like 1, 2, 3 ...

@arikfr
Copy link
Member

arikfr commented Oct 15, 2015

@tjwudi this is ready for merge, right?

@tjwudi
Copy link
Contributor Author

tjwudi commented Oct 15, 2015

Yup, for me it's ready :) @arikfr

@ghost
Copy link

ghost commented Oct 20, 2015

👍

1 similar comment
@shenyubao
Copy link

👍

@Danier-Evens
Copy link

@arikfr when to merge?

@tjwudi
Copy link
Contributor Author

tjwudi commented Oct 26, 2015

@Danier-Evens probably in this week. @arikfr sent me an email saying that he is kinda busy working on something else. :)

@Danier-Evens
Copy link

@tjwudi @arikfr OK! Thanks.

max: moment.max.apply(undefined, _.map(allSeries, function (series) {
return moment.max(_.pluck(series.data, 'x'));
}))
};
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure this is working?

I've just tried it, and I get min as 1/1/1970 and max as current date.

Copy link
Member

Choose a reason for hiding this comment

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

I just noticed that you upgraded moment. I guess that's the issue.

@arikfr arikfr changed the title Date range selector support Feature: date range selector support for charts Nov 3, 2015
arikfr added a commit that referenced this pull request Nov 3, 2015
Feature: date range selector support for charts
@arikfr arikfr merged commit 67aecc0 into getredash:master Nov 3, 2015
@arikfr
Copy link
Member

arikfr commented Nov 3, 2015

Took me a while, but it's finally merged.

👍

@tjwudi
Copy link
Contributor Author

tjwudi commented Nov 3, 2015

Awesome! Thanks!

@tjwudi tjwudi mentioned this pull request Nov 13, 2015
dairyo pushed a commit to KiiCorp/redash that referenced this pull request Mar 1, 2019
…selector

Feature: date range selector support for charts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants