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

[filter bar] Add aliasing. Closes #5194 #5268

Merged
merged 4 commits into from
Nov 5, 2015
Merged

Conversation

jbudz
Copy link
Member

@jbudz jbudz commented Nov 2, 2015

image

Closes #5194

@rashidkpc
Copy link
Contributor

I don't like using a key on the JSON for the alias. Can we add an input field that attaches the alias to _meta.alias?

@jbudz
Copy link
Member Author

jbudz commented Nov 2, 2015

image

@tbragin
Copy link
Contributor

tbragin commented Nov 4, 2015

When the label set is too short, the Edit icon is no longer visible in the filter bar. This problem is actually not specific to adding aliases, even adding a filter that generates a short default description will suffer from this problem, but aliases will in particular bring this out.
2015-11-03 16_38_11

function convertToEditableFilter(filter) {
var model = _.cloneDeep(filter);

var filterType;
Copy link
Contributor

Choose a reason for hiding this comment

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

This variable isn't used anymore, so you can ditch this.

@epixa
Copy link
Contributor

epixa commented Nov 4, 2015

None of my feedback impacts the actual functionality of this feature. As far as I can tell, the only thing that really must be fixed before this can be merged is the min-width bug that @tbragin pointed out here.

@rashidkpc rashidkpc assigned jbudz and unassigned rashidkpc Nov 4, 2015
@jbudz jbudz assigned epixa and unassigned jbudz Nov 4, 2015
@jbudz
Copy link
Member Author

jbudz commented Nov 4, 2015

Zooming out to less than 50% can help reproduce the issue with the icon dropping off that the last commit addresses.

@tbragin
Copy link
Contributor

tbragin commented Nov 5, 2015

@jbugz confirmed the issue as fixed by the last commit. i zoomed all the way out to 25% :) LGTM

@epixa
Copy link
Contributor

epixa commented Nov 5, 2015

LGTM

@epixa epixa assigned jbudz and unassigned epixa Nov 5, 2015
@rashidkpc
Copy link
Contributor

Merge it!

jbudz added a commit that referenced this pull request Nov 5, 2015
[filter bar] Add aliasing.  Closes #5194
@jbudz jbudz merged commit e263531 into elastic:master Nov 5, 2015
@acs
Copy link

acs commented Nov 5, 2015

Super!

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.

5 participants