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

Add a feature for custom panel titles #14831

Merged

Conversation

stacey-gammon
Copy link
Contributor

@stacey-gammon stacey-gammon commented Nov 7, 2017

Release note: Introduces the ability to specify custom titles, or remove the title entirely, for individual dashboard panels. Just hit the reset link to restore the title to it's original value.

Screenshots:
With no customization:
screen shot 2017-11-15 at 4 11 40 pm

Customization process:
screen shot 2017-11-15 at 4 12 01 pm
screen shot 2017-11-15 at 4 12 08 pm

With customized, or removed, titles:
screen shot 2017-11-15 at 4 13 04 pm

@trevan
Copy link
Contributor

trevan commented Nov 7, 2017

Is it possible to blank the title?

@stacey-gammon
Copy link
Contributor Author

Is it possible to blank the title?

@trevan - yep, you just delete all the text. I wonder if we could make this a little clearer though. maybe an X button, or when you delete all the text, use placeholder text that says no title.

When there is no title the panel header hides in view mode, to give more space to the visualization underneath, and the actions menu shows up on hover. This is causing it to overlay the legend toggle a little bit but I think it won't be as bad once #14811 is submitted, which adds some padding.

screen shot 2017-11-07 at 5 15 18 pm

}
return (
<div
data-test-subj="dashboardPanelTitleInputMenuItem"
Copy link
Contributor

Choose a reason for hiding this comment

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

You can add style={{ padding: 16 }} to this container to fix the padding for now. @snide Any ideas for a component which can do this for us?

Copy link
Contributor

Choose a reason for hiding this comment

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

I've thought of making a padding size="m" component similar to spacer. Unfortunately with the way context / popovers can be different it's not easy to come up with a one size fits all solution.

Copy link
Contributor

Choose a reason for hiding this comment

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

Worst-case scenario, how about a borderless, shadowless Panel?

@stacey-gammon
Copy link
Contributor Author

Here's how the expand floater looks with the new padding stuff. Still overlays it a little bit, but looks better.

screen shot 2017-11-09 at 10 33 05 am

@stacey-gammon
Copy link
Contributor Author

stacey-gammon commented Nov 9, 2017

Actually, having no title now looks a little weird because there is padding everywhere but the top:

screen shot 2017-11-09 at 10 40 01 am

I'm a little worried users won't like the margins + padding taking up so much space away from their visualizations.

@snide
Copy link
Contributor

snide commented Nov 9, 2017

@stacey-gammon I hadn't thought about background coloring. Do you think we could maybe dedicate a couple hours to pair through this stuff. I really don't have the knowledge of depth of configuration that some of these panels can get in, and all of its fixable (for example, putting the padding on the viz itself so the coloring can be flush) but I'll likely run into this stuff a lot without a suped up dash to test locally.

Is there a premade set of saved objects you use?

@stacey-gammon
Copy link
Contributor Author

Yea, lets sync tomorrow @snide.. Time series visual builder can have background color, and maps, are the only two I'm aware of that might look silly like that.

@stacey-gammon
Copy link
Contributor Author

@snide - I think I fixed up all the padding. Pushing it into the visualization restores the padding but keeps the colors flush to the edge of the panel. Except for tile maps which I had to override... and some reason region maps don't need it? That was strange (cc @thomasneirynck).

Anyway, seems to be doing what we want now, though I made the padding 8px instead of 16px. Still helps with the expand toggle.
screen shot 2017-11-14 at 3 09 47 pm
screen shot 2017-11-14 at 3 15 07 pm

@snide
Copy link
Contributor

snide commented Nov 14, 2017

@stacey-gammon awesome, thank you! looks great.

@nreese
Copy link
Contributor

nreese commented Nov 15, 2017

Looks like the menu is getting cut off on the right side

screen shot 2017-11-15 at 6 41 19 am

@stacey-gammon
Copy link
Contributor Author

Ah, good call @nreese, should be fixed now:

screen shot 2017-11-15 at 9 46 00 am

@@ -26,6 +29,7 @@ function getProps(props = {}) {

beforeAll(() => {
store.dispatch(updateViewMode(DashboardViewMode.EDIT));
store.dispatch(setPanels([{ panelIndex: 'foo1' }]));
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this doing? Why foo1?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's setting up redux so that the dashboard will be rendered with a single panel with a panel id/index of foo1 . foo1 could be anything as long as it matches the defaultTestProps panel above. Without this call it'll break because you will be passing in props that say render panel foo1 but that panel won't exist in the store, so I'll get an error like Cannot read property 'title' of undefined.

Copy link
Contributor

Choose a reason for hiding this comment

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

sorry, I did not see this was a test file. Disregard the questions

];
if (!this.props.isExpanded) {
items.push(<DeleteMenuItem key="3" onDeletePanel={this.onDeletePanel} />);
mainPanelMenuItems.push({
name: 'Delete visualization',
Copy link
Contributor

Choose a reason for hiding this comment

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

Delete visualization is a little ambiguous. Does it delete the visualization from the dashboard or delete the visualization from kibana entirely? Maybe keep the old text Delete from dashboard?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, this name change wasn't intentional. I'll put back the old variation!

@nreese
Copy link
Contributor

nreese commented Nov 15, 2017

One more thing - would it be possible to color the caret (little triangle thingy) in the menu? It looks a little funny when the top of the menu is grey and the caret is still white.

screen shot 2017-11-15 at 8 37 47 am

@stacey-gammon
Copy link
Contributor Author

Looks like the popover component has the ability to set that by using withTitle, but I'm not totally sure what the title should be for the main panel - Panel settings and Panel options? Or maybe since right now it's just the panel title I should do Panel options for the main panel and Edit title
for the link and second panel? Thoughts? Long term, we do want Panel options because more stuff will go in there (e.g. custom time range). So maybe worth figuring out the options wording now instead of using panel title.

screen shot 2017-11-15 at 11 04 10 am

screen shot 2017-11-15 at 11 04 07 am

@nreese
Copy link
Contributor

nreese commented Nov 15, 2017

Maybe label the top menu Panel or Panel menu?

I think it makes sense to leave the secondary menu labeled as Panel options since more stuff will be added to it.

@cjcenizal
Copy link
Contributor

How about naming the first panel "Options" and the second panel "Customize panel"? That makes more sense to me since the first panel has a bunch of somewhat-disconnected choices, so an ambiguous term like "Options" applies, whereas the second panel has a more specific purpose (to customize appearance).

@nreese
Copy link
Contributor

nreese commented Nov 15, 2017

Looks like there is no padding on Customize panel menu

screen shot 2017-11-15 at 12 45 14 pm

@cjcenizal
Copy link
Contributor

@nreese I think we should add a version of PanelSimple which has padding but no border or shadow around it. That would solve the problem here and it would also be something we can use in the EUI Framework. @snide does this sound good to you for a short-term solution?

@stacey-gammon
Copy link
Contributor Author

@nreese can you try refreshing the browser or restarting kibana? Maybe you switched branches and it didn't pick up the updated style sheet? Should still have the padding on it.

screen shot 2017-11-15 at 3 31 06 pm

Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

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

server restart fixed everything and the padding is there.

lgtm

@stacey-gammon
Copy link
Contributor Author

stacey-gammon commented Nov 15, 2017

Failed on #14920

jenkins, test this

Copy link
Contributor

@kobelb kobelb left a comment

Choose a reason for hiding this comment

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

LGTM

@stacey-gammon stacey-gammon merged commit c2c1b51 into elastic:master Nov 16, 2017
stacey-gammon added a commit to stacey-gammon/kibana that referenced this pull request Nov 16, 2017
* Add a feature for custom panel titles

* Add tests and put back data-test-subjs

* sync with master and add padding to form

* UI/UX cleanup

- add enter on close functionality
- make reset title a link instead of a button
- Push css to visualizations instead of the panel. This means
background colors will be flush to the panel.  Override for tile maps
which apparently need it (yet region maps don’t for some reason??)

* Fix refactor miss from merge

* whoops, put block display back to make link fall to bottom

* Undo accidental delete visualization name change

* Color top pop over arrow correctly

* Use naming Options and Customize Panel

* update jest snapshot

* Use custom panels for data-title attributes
stacey-gammon added a commit that referenced this pull request Nov 16, 2017
* Add a feature for custom panel titles

* Add tests and put back data-test-subjs

* sync with master and add padding to form

* UI/UX cleanup

- add enter on close functionality
- make reset title a link instead of a button
- Push css to visualizations instead of the panel. This means
background colors will be flush to the panel.  Override for tile maps
which apparently need it (yet region maps don’t for some reason??)

* Fix refactor miss from merge

* whoops, put block display back to make link fall to bottom

* Undo accidental delete visualization name change

* Color top pop over arrow correctly

* Use naming Options and Customize Panel

* update jest snapshot

* Use custom panels for data-title attributes
chrisronline pushed a commit to chrisronline/kibana that referenced this pull request Dec 1, 2017
* Add a feature for custom panel titles

* Add tests and put back data-test-subjs

* sync with master and add padding to form

* UI/UX cleanup

- add enter on close functionality
- make reset title a link instead of a button
- Push css to visualizations instead of the panel. This means
background colors will be flush to the panel.  Override for tile maps
which apparently need it (yet region maps don’t for some reason??)

* Fix refactor miss from merge

* whoops, put block display back to make link fall to bottom

* Undo accidental delete visualization name change

* Color top pop over arrow correctly

* Use naming Options and Customize Panel

* update jest snapshot

* Use custom panels for data-title attributes
@stacey-gammon stacey-gammon deleted the feature/custom-panel-titles branch September 21, 2018 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Dashboard Dashboard related features v6.1.0 v7.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants