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

[Dashboard] Implement custom title for panels. #5607

Closed
wants to merge 18 commits into from
Closed

[Dashboard] Implement custom title for panels. #5607

wants to merge 18 commits into from

Conversation

acs
Copy link
Contributor

@acs acs commented Dec 9, 2015

Fix #3393

@elasticsearch-release
Copy link

Jenkins standing by to test this. If you aren't a maintainer, you can ignore this comment. Someone with commit access, please review this and clear it for Jenkins to run; then say 'jenkins, test it'.

@rashidkpc
Copy link
Contributor

I like the functionality here, there's a few things that would need to change to get it merge-able:

  • Store the title on $scope.uiState using the set/get methods.
  • Instead of a button, make the title clickable. Perhaps a hover with an icon to indicate it is editable. When the title is clicked change it to an editable input with the existing title already filled.
  • Add confirm, cancel and reset icons as buttons. Note the markup used in the dashboard query bar for getting them to flow nicely together.

@LeeSyd
Copy link

LeeSyd commented Dec 9, 2015

Different organisation but obviously similar requirements. Have viewed the Youtube video linked on the referenced article and looks good in terms of labeling aggregations/metrics. Would still like to extend this to cover the title of the saved visualisation as well if possible?

image

@rashidkpc
Copy link
Contributor

This pull covers your "Also requested" not the "covered by update"

@acs
Copy link
Contributor Author

acs commented Dec 9, 2015

@rashidkpc I think also in editing the title just clicking on it, but I was afraid that the normal user does not find the feature. But adding a hover with an icon is a good idea. Is there some sample UI in KIbana I can use as reference?

I will try to fix the other issues in order to get a merge-able PR. Thanks!

@rashidkpc
Copy link
Contributor

Nothing currently, but we're moving in that direction. I'd say any time someone hovers the title bar of the panel, show the icon.

@acs
Copy link
Contributor Author

acs commented Dec 9, 2015

Ok, I will play with the idea of showing the icon in hover case. It is the most effective UI. Also, I was thinking in just changing the full title with the edit input when in editing mode. So the title gets editable. Time to play :)

@acs
Copy link
Contributor Author

acs commented Dec 10, 2015

@rashidkpc I have implemented your suggestions. Time to wait now for your review to iterate over the PR.

@rashidkpc
Copy link
Contributor

jenkins, test it

<span ng-if="panel.customTitle">{{panel.customTitle}}</span>
<span ng-hide="panel.customTitle">{{savedObj.title}}</span>
</a>
<input size="10" ng-if="showFormTitle" text="title" ng-model="panel.customTitle" ng-show="chrome.getVisible() && editUrl">
Copy link
Contributor

Choose a reason for hiding this comment

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

  • This needs the proper bootstrap form classes added to it. See examples in the query bar.
  • Requires editUrl in order to be shown?
  • Just call it title instead of customTitle
  • Don't set size=10, use css to control the input width. See bootstrap way of doing this.
  • The ng-click shouldn't fire if !chrome.getVisible()

@rashidkpc
Copy link
Contributor

I don't like the syncing back and forth of $scope.panel and $scope.uiState here, it would be more graceful to persist uiState on the dashboard. Thoughts @w33ble?

Proper bootstrap form classes added.
Remove not needed editUrl.
Use title as param name.
Remove arrow syntax.
@acs
Copy link
Contributor Author

acs commented Dec 11, 2015

@rashidkpc new version uploaded fixing some of your review comments.

Some doubts/issues:

  • I added editUrl because I thought this control that the dashboard is in edit mode. If it is not in edit mode, the title should not be editable. I have removed this check.
  • I have tried to layout the form as closer as possible to the query bar, But query bar is inside a and the styles are different. The mix between bootstrap and flex layout make some strange behaviors when for example, trying to use class="form-control" in the input text.
  • The ng-click shouldn't fire if !chrome.getVisible(): In this state, is it needed the title editor at all? Not sure yet what this checking control.

@w33ble
Copy link
Contributor

w33ble commented Dec 14, 2015

I don't like the syncing back and forth of $scope.panel and $scope.uiState here

The syncing itself isn't toublesome - it's cumbersome, sure, but that's pretty much what needs to happen. Ideally though, this code should never set panel.title by hand, it should just change the uiState's title value and let the syncUI handler take care of the rest.

@acs
Copy link
Contributor Author

acs commented Feb 11, 2016

@rashidkpc I think this version follows better Kibana style. Just pending fix the title of the panel so it uses the custom title. And next step should be adding the tests for this new feature?

@rashidkpc
Copy link
Contributor

@acs Visually this looks good now! Tests would seal the deal!

@acs
Copy link
Contributor Author

acs commented Feb 11, 2016

@rashidkpc great! I have created tests in the past for the custom label feature. But this one includes more logic. I should include Selenium tests for the GUI? Also, with mocha tests for the backend functionality. Any clues will be greatly appreciated. I will start working on tests asap.

@rashidkpc
Copy link
Contributor

Selenium tests would be neccesary for a change of this nature, yes. I also noticed with the last merge this changes all of the ES6 code back to ES5, everything here should be ES6.

@elasticsearch-release
Copy link

Jenkins standing by to test this. If you aren't a maintainer, you can ignore this comment. Someone with commit access, please review this and clear it for Jenkins to run; then say 'jenkins, test it'.

@rashidkpc rashidkpc removed their assignment Mar 9, 2016
@rashidkpc
Copy link
Contributor

Going to un-assign this for now, @acs if you make any updates please @rashidkpc ping me

@acs
Copy link
Contributor Author

acs commented Mar 10, 2016

@rashidkpc really busy working in our full retrieval and analytics platform. But creating these tests is on my high priority list. As soon as I have results, I will ping you in this ticket. My slot time for working on this should be next week probably.

@acs
Copy link
Contributor Author

acs commented Mar 15, 2016

@rashidkpc I have started to work in creating the Selenium testing. I am following https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#running-browser-automation-tests Any special recommendation if needed is welcomed.

@acs
Copy link
Contributor Author

acs commented Mar 15, 2016

@rashidkpc just getting familiar with intern, I can not see the tests for the dashboard (the custom title is something specific of the dashboards).

https://github.com/elastic/kibana/tree/master/test/functional/apps

Dashboard app does not have tests?

@rashidkpc
Copy link
Contributor

It does not currently have selenium tests, correct, we're working on it. @LeeDr any idea how that looks right now?

@LeeDr
Copy link
Contributor

LeeDr commented Mar 16, 2016

I have a dashboard_page.js that should be on a PR soon. Right now it only has a couple of methods like getting the titles of the visualizations on a dashboard. And I have some tests that include dashboards but they're integration tests with beats products so they're a little farther out. In any case, I'd be happy to help you with these tests.

@acs
Copy link
Contributor Author

acs commented Mar 16, 2016

@LeeDr great news! The tests should be fairly simple. Fill the input text with some title sample, and then interact with the three buttons and check the results. What are the best approach to implement them? Start from your PR and implement using it?

@LeeDr
Copy link
Contributor

LeeDr commented Mar 17, 2016

Currently, the visualize tests all clean up before and after themselves, so there's no saved visualizations to add to a dashboard. I'm going to submit a PR soon (today or tomorrow I hope) that changes the visualize tests to start with an empty kibana index but leave all the visualization there when done. And I have a new Dashboard suite that adds all those visualizations to a dashboard, saves it, loads it, etc. I'll mention you @acs on that PR so you'll see it. You can tack your tests onto that new dashboard test.

@acs
Copy link
Contributor Author

acs commented Mar 17, 2016

@LeeDr cool!

@acs
Copy link
Contributor Author

acs commented Mar 18, 2016

@rashidkpc So, until there are tests for the Dashboard in Kibana, this PR is blocked? Maybe we can add unit testing, merge the PR and add the tests once the Dashboard tests are ready for production? Just to plan my mind and my work :) I am ok with both plans.

@spalger
Copy link
Contributor

spalger commented Apr 5, 2016

jenkins, test it

@rashidkpc
Copy link
Contributor

@LeeDr how are we looking on minimal Dashboard tests?

@LeeDr
Copy link
Contributor

LeeDr commented Apr 12, 2016

@rashidkpc I created #6576 for initial Dashboard tests.
I had a discussion with @tylersmalley and we agreed that a better approach than what I've done in that PR would be to add functionality to;
[ ] save or export each viz
[ ] test the exported JSON
[ ] have the dashboard test load saved vizs

What I've done so far in that PR is to change the viz tests to leave the saved visualizations by not reloading the empty .kibana index at the start of each test (which does save some test time). And then the Dashboard tests depend on those saved viz being there (not the most desired approach).

So that's why there's no Review label on that PR yet. I'll try to rework it ASAP.

@panda01
Copy link
Contributor

panda01 commented May 24, 2016

I'm not a fan of this implementation. IMO, this should be it's own directive. That simply takes an ng model and changes the necessary variable. that would remove the need for integration tests on dashboard which is the hold up for this, and make it so we can use it everywhere, and there are a few places i could imagine this would be useful.

I'm going to take a crack at this.

@panda01
Copy link
Contributor

panda01 commented Jun 8, 2016

Closed in favor of the above PR

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.

None yet

9 participants