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

Branding fixes for dashboard, loader and space selector #60073

Merged
merged 9 commits into from
Mar 18, 2020
Merged

Branding fixes for dashboard, loader and space selector #60073

merged 9 commits into from
Mar 18, 2020

Conversation

snide
Copy link
Contributor

@snide snide commented Mar 12, 2020 •
edited
Loading

Summary

Fixes #59955

Some more cleanup for branding.

cc @alexfrancoeur @cchaos @ryankeairns

Fullscreen exit on dashboard

image

image

Correct colors in dark mode loader

image

Spaces screen now has correct logo

image

@snide snide added release_note:skip Skip the PR/issue when compiling release notes v7.7.0 labels Mar 12, 2020
@snide snide requested a review from alexfrancoeur March 12, 2020 23:26
@snide snide requested review from a team as code owners March 12, 2020 23:26
alexfrancoeur
alexfrancoeur approved these changes Mar 12, 2020
View reviewed changes
Copy link

@alexfrancoeur alexfrancoeur left a comment

Choose a reason for hiding this comment

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

:shipit:

legrego
legrego approved these changes Mar 13, 2020
View reviewed changes
Copy link
Member

@legrego legrego left a comment

Choose a reason for hiding this comment

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

Spaces changes LGTM

@cchaos
Copy link
Contributor

cchaos commented Mar 13, 2020 •
edited
Loading

I 😍 this PR. Looks really great!

I do have a couple concerns with the new exit full screen button. Don't get me wrong, I do really like the way it looks. So much classier. My worry is with it functionally.

Screen Shot 2020-03-13 at 11 35 14 AM

  1. How much larger is this than the old one? My concern over size is that since it floats on top of the dashboard it could cover some vital bits of the visualizations. We're not going to solve the floating/covering issue in this PR but we can minimize the impact by reducing the size.
  2. All it says is "Powered by". There's no mention of Full Screen here, so my expectation was that clicking the x would remove this little popup and not exit full screen. I think it could be really confusing. Perhaps if the x was actually the minimize icon? Or something that indicates exiting full screen.

@snide
Copy link
Contributor Author

snide commented Mar 13, 2020

All it says is "Powered by". There's no mention of Full Screen here, so my expectation was that clicking the x would remove this little popup and not exit full screen. I think it could be really confusing. Perhaps if the x was actually the minimize icon? Or something that indicates exiting full screen.

That's a good point. I can also see about dropping the size down a bit.

lizozom
lizozom approved these changes Mar 15, 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.

Changes tested and working.
However, I do want to say that the new full screen exit button design, doesn't look like intuitive to me.
It looks more like a notification that needs to be dismissed.

image

Technically however, LGTM

@snide
Copy link
Contributor Author

snide commented Mar 18, 2020

@cchaos Went smaller. Flipped the colors so it would be noticible now that the surrounding whitespace doesn't key it out. Text and icon changed as well.

image

cchaos
cchaos approved these changes Mar 18, 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.

The full screen button is more more obvious what it does now and users will be happier with the condensed size. 👍 🚀

src/core/server/rendering/views/styles.tsx Show resolved Hide resolved
@snide
Copy link
Contributor Author

snide commented Mar 18, 2020

jenkins test this

@snide
Copy link
Contributor Author

snide commented Mar 18, 2020 •
edited
Loading

Merging this one on green.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@snide snide merged commit 52dd5e0 into elastic:master Mar 18, 2020
@snide snide deleted the brand/fix branch March 18, 2020 17:15
@snide snide mentioned this pull request Mar 18, 2020
Merged
jloleysens added a commit that referenced this pull request Mar 18, 2020
@jloleysens
Merge branch 'app/painless' of github.com:elastic/kibana into app/pai…
8733480 
…nless

* 'app/painless' of github.com:elastic/kibana: (64 commits)
  Fix filter scope in bool query (#60488)
  change index pattern id to be the same as index pattern title (#60436)
  [Endpoint] resolver v1 events (#59233)
  Branding fixes for dashboard, loader and space selector (#60073)
  skip flaky suite (#60535)
  [SIEM][Detection Engine] Fixes bug with timeline templates not working
  Fixed errors which are happening if switch between alert types (#60453)
  [EPM] Add mapping field types to index template generation v2 (#60266)
  [NP] Cutover ensureDefaultIndexPattern to kibana_utils (#59895)
  Closes #60265. Adds Beta badge to service map (#60482)
  [Visualize] Duplicated query filters in es request (#60106)
  [ML] Disable functional transform tests
  Fixes to service map single node banner (#60072)
  [Uptime] replace fetch with kibana http (#59881)
  Upgrade @types/node to match Node.js runtime (#60368)
  [License Management] NP migration (#60250)
  Fix create alert button from not showing in alerts list (#60444)
  [SIEM][Case] Update connector through flyout (#60307)
  add data-test-subj where possible on SO management table (#60226)
  Enforce `required` presence for value/key validation of `recordOf` and `mapOf`. (#60406)
  ...
snide added a commit that referenced this pull request Mar 19, 2020
@snide
Branding fixes for dashboard, loader and space selector (#60073) (#60539
6768356 
)
jloleysens added a commit to jloleysens/kibana that referenced this pull request Mar 19, 2020
@jloleysens
Merge remote-tracking branch 'upstream/app/painless' into minor-refactor
3f00238 
* upstream/app/painless: (66 commits)
  Another i18n issue
  Fix i18n
  Fix filter scope in bool query (elastic#60488)
  change index pattern id to be the same as index pattern title (elastic#60436)
  [Endpoint] resolver v1 events (elastic#59233)
  Branding fixes for dashboard, loader and space selector (elastic#60073)
  skip flaky suite (elastic#60535)
  [SIEM][Detection Engine] Fixes bug with timeline templates not working
  Fixed errors which are happening if switch between alert types (elastic#60453)
  [EPM] Add mapping field types to index template generation v2 (elastic#60266)
  [NP] Cutover ensureDefaultIndexPattern to kibana_utils (elastic#59895)
  Closes elastic#60265. Adds Beta badge to service map (elastic#60482)
  [Visualize] Duplicated query filters in es request (elastic#60106)
  [ML] Disable functional transform tests
  Fixes to service map single node banner (elastic#60072)
  [Uptime] replace fetch with kibana http (elastic#59881)
  Upgrade @types/node to match Node.js runtime (elastic#60368)
  [License Management] NP migration (elastic#60250)
  Fix create alert button from not showing in alerts list (elastic#60444)
  [SIEM][Case] Update connector through flyout (elastic#60307)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexfrancoeur alexfrancoeur alexfrancoeur approved these changes

@legrego legrego legrego approved these changes

@mshustov mshustov mshustov approved these changes

@lizozom lizozom lizozom approved these changes

@cchaos cchaos cchaos approved these changes

Assignees
No one assigned
Labels
release_note:skip Skip the PR/issue when compiling release notes v7.7.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Branding tweaks
7 participants
@snide @cchaos @kibanamachine @lizozom @mshustov @legrego @alexfrancoeur