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

rd_ui: sync filters with location.search [closes #146] #253

Merged
merged 2 commits into from
Aug 7, 2014

Conversation

amirnissim
Copy link
Contributor

closes #146

var once = true;

function fromLocation() {
var searchFilters = angular.fromJson($location.search().filters);
Copy link
Member

Choose a reason for hiding this comment

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

Why JSON and not separate parameters? Separate parameters will be more user friendly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we need to serialize and deserialize information like

filters = {
  filter1: val1,
  filter2: val2
}

what do you suggest instead?

Copy link
Member

Choose a reason for hiding this comment

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

maybe prefix filter names with filter_ (so filter named country will become filter_country)?

Copy link
Member

Choose a reason for hiding this comment

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

(you need to use the filter.name sans the ::filter part, you can use the QueryResult.getColumnNameWithoutType function for this)

@amirnissim
Copy link
Contributor Author

update PR with tests and more. need your feedback @arikfr .

@ranbena I borrowed eme_services.js from lite. Will be nice to have a shareable angular.me repo 👍 (we can make it a bower package and then use it in multiple ng projects)

_.each(filters, function(filter) {
if (filter.current) {
current[filter.friendlyName] =
_.isArray(filter.current) ? filter.current[0] : filter.current;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@arikfr not sure about this. will probably fail for multi-filters. ideas?

Copy link
Member

Choose a reason for hiding this comment

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

indeed will fail for multi-filters. how about serializing the array as:

serialize: ','.join(filter.current)
deserialize: filter.current.split(',')

?

Copy link
Member

Choose a reason for hiding this comment

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

obviously not bullet proof (or comma proof), but can take us very far :)

@arikfr
Copy link
Member

arikfr commented Aug 7, 2014

@amirnissim why taking it further and release each part of eme_services as reusable package that others can use too? or the stuff there are too specific for our needs?

* modified: jQuery.isArray => angular.isArray
* modified: jQuery.isPlainObject => angular.isObject
*/
extend: function() {
Copy link
Member

Choose a reason for hiding this comment

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

Why not use _.extend?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ranbena?
I think jquery's version is the only one with 'deep' extend

@amirnissim
Copy link
Contributor Author

I reverted the PR back to pre URLService.
It was getting too hairy. @arikfr let's go with JSON in the url for now and improve it in a followup.

@arikfr
Copy link
Member

arikfr commented Aug 7, 2014

@amirnissim I thought about going with JSON and then changing, but then we will have broken links. Why did you feel it was becoming "hairy"?

@amirnissim
Copy link
Contributor Author

I thought about going with JSON and then changing, but then we will have broken links. Why did you feel it was becoming "hairy"?

why broken links? I tests with filters and multifilters and it worked (angular URLencodes the value you pass to $location.search)

the util I copied to support URL params does not support arrays. adding good support is not trivial (see https://github.com/EverythingMe/redash/tree/URLService)

@arikfr
Copy link
Member

arikfr commented Aug 7, 2014

I meant that when we move to friendlier implementation, these links will break. But yalla, let's deal with it then.

arikfr added a commit that referenced this pull request Aug 7, 2014
rd_ui: sync filters with location.search [closes #146]
@arikfr arikfr merged commit 8940d66 into master Aug 7, 2014
@arikfr arikfr deleted the 146-filter-sync branch April 1, 2015 14:26
jezdez pushed a commit to jezdez/redash that referenced this pull request Nov 1, 2017
convert angle brackets in strings to html entities (re getredash#222)
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.

Changing filters should update the query string and filters should react to query string
2 participants