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.

Sign up for GitHub

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

Jump to bottom

Style dashboard buttons the same way #63403

Merged
merged 3 commits into from
Apr 16, 2020
Merged

Style dashboard buttons the same way #63403

merged 3 commits into from
Apr 16, 2020

Conversation

ryankeairns
Copy link
Contributor

@ryankeairns ryankeairns commented Apr 13, 2020 •
edited
Loading

Fixes #62070

Summary

Several recent changes were made to Dashboard placing more emphasis on the 'Create new' action. In the end, we ended up with some discrepancies and competing visual aspects.

To tighten up the design and visual cues, this PR:

  1. Uses the same plusInCircle EUI icon on the left side of each button
  2. Reduces the visual weight of the button in the empty dashboard panel by using the hollow alternative
  3. Sets the text size in the menu button to the default EUI size (16px). I too would prefer it were smaller in this case, but that is something we should take up with EUI (i.e. reducing the font size on small buttons to 14px) since all other Kibana buttons use 16px. Further, the smaller value was actually calculating out to 13.333px.

Before

Screenshot 2020-04-13 14 42 07

After

Screenshot 2020-04-13 15 25 54

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@ryankeairns ryankeairns added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.8.0 labels Apr 13, 2020
@ryankeairns ryankeairns requested a review from a team April 13, 2020 20:28
@ryankeairns ryankeairns requested review from a team as code owners April 13, 2020 20:28
@botelastic botelastic bot added the Feature:Embedding Embedding content via iFrame label Apr 13, 2020
ThomThomson
ThomThomson approved these changes Apr 14, 2020
View reviewed changes
Copy link
Contributor

@ThomThomson ThomThomson left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Tested locally on Chrome, all buttons work as they should, and match much better.

Should we be thinking about re-wording it from 'Create new' to something more specific like 'Create panel' or something else while we're here?

Kibana app code LGTM

lizozom
lizozom reviewed Apr 14, 2020
View reviewed changes
src/plugins/navigation/public/top_nav_menu/top_nav_menu_item.tsx
@@ -49,7 +49,7 @@ export function TopNavMenuItem(props: TopNavMenuData) {
};

const btn = props.emphasize ? (
<EuiButton {...commonButtonProps} size="s" fill style={{ fontSize: 'smaller' }}>
<EuiButton {...commonButtonProps} size="s" fill>
Copy link
Contributor

Choose a reason for hiding this comment

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

Changing this to size "S" increases the height of the nav row and also creates a weird visual effect when switching edit mode on and off. Is that OK?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With Devon's original PR (adding this button) we accepted the height change between view and edit modes. This particular change here only affects the font-size and has no impact on the overall button height.

This situation will be addressed when we move to the new header design where the menus are anticipated to move up into the header area - the plan as of now is for a double header, like GitHub, and will better accommodate buttons in the menu layout.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lizozom given that explanation, can you approve this PR? Thanks!

cchaos
cchaos approved these changes Apr 14, 2020
View reviewed changes
Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

SASS and EUI stuffs LGTM! 💯

src/plugins/navigation/public/top_nav_menu/top_nav_menu_item.tsx
@@ -49,7 +49,7 @@ export function TopNavMenuItem(props: TopNavMenuData) {
};

const btn = props.emphasize ? (
<EuiButton {...commonButtonProps} size="s" fill style={{ fontSize: 'smaller' }}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you!

@ryankeairns
Copy link
Contributor Author

Should we be thinking about re-wording it from 'Create new' to something more specific like 'Create panel' or something else while we're here?

We do try to use a verb + noun combination and this does feel like an outlier. The tricky part here is that we're trying to emphasize that you are creating new content as opposed to the existing 'Add panel' functionality which serves a different purpose. In that way, having both Create panel and Add panel presents another problem. That said...

There is one more iteration coming for this button that will allow us to clean this up and that is defaulting the button to open Lens. The plan is to make this a split button with the main piece (Create new Lens; Open Lens; etc) opening directly to Lens and the second piece (likely an icon button with arrow down) will open the viz type picker.

Once Lens is a bit more mature, we'll make that switch.

@ryankeairns
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

lizozom
lizozom approved these changes Apr 16, 2020
View reviewed changes
Copy link
Contributor

@lizozom lizozom left a comment

Choose a reason for hiding this comment

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

LGTM

@ryankeairns ryankeairns merged commit 373d492 into elastic:master Apr 16, 2020
@ryankeairns ryankeairns mentioned this pull request Apr 16, 2020
Merged
ryankeairns added a commit to ryankeairns/kibana that referenced this pull request Apr 16, 2020
@ryankeairns @elasticmachine
Style dashboard buttons the same way (elastic#63403)
311eb03 
* Stylize dashboard buttons the same way

* update snapshots

Co-authored-by: Elastic Machine <[email protected]>
ryankeairns added a commit that referenced this pull request Apr 16, 2020
@ryankeairns @elasticmachine
Style dashboard buttons the same way (#63403) (#63697)
959bf00 
* Stylize dashboard buttons the same way

* update snapshots

Co-authored-by: Elastic Machine <[email protected]>

Co-authored-by: Elastic Machine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ThomThomson ThomThomson ThomThomson approved these changes

@lizozom lizozom lizozom approved these changes

@cchaos cchaos cchaos approved these changes

Assignees
No one assigned
Labels
Feature:Embedding Embedding content via iFrame release_note:skip Skip the PR/issue when compiling release notes v7.8.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Create button in empty state for dashboard
6 participants
@ryankeairns @kibanamachine @cchaos @lizozom @ThomThomson @elasticmachine