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

[Enterprise Search] Design Pass: Role mappings #96882

Merged
merged 6 commits into from
Apr 13, 2021
Merged

[Enterprise Search] Design Pass: Role mappings #96882

merged 6 commits into from
Apr 13, 2021

Conversation

scottybollinger
Copy link
Contributor

@scottybollinger scottybollinger commented Apr 12, 2021 •
edited
Loading

Summary

This PR completes the checklist items for Role mappings here.

Changes requested:

  • For the "No role mappings yet" message, add a full-width "color=subdued" and "padding=l" panel around the entire message (icon, title, message/description, and button)
  • For the "No role mappings yet" message, change the button color to blue
  • When creating a new role mapping, all panels should be "color=subdued"
  • For the Users & roles screen (after a mapping has been added), the "Add mapping" button should be blue
    • If possible, for the table, vertically align all content within each table cell to the top. (ping me if this isn't clear)

Went ahead and applied the same changes to App Search while I was here. I only did the screenshot for App Search for the table cell alignment since this is a shared component.

Best to review each commit with whitespace changes hidden.

App Search

Before After
as-rm-empty-before as-rm-empty-after
as-rm-new-before as-rm-new-after
as-rm-list-before as-rm-list-after
as-rm-table-align-before as-rm-table-align-after

Workplace Search

Before After
rm-empty-before rm-empty-after
rm-new-before rm-new-after
rm-list-before rm-list-after

Checklist

@scottybollinger scottybollinger added Feature:Plugins release_note:skip Skip the PR/issue when compiling release notes v7.13.0 auto-backport Deprecated - use backport:version if exact versions are needed labels Apr 12, 2021
@scottybollinger scottybollinger requested review from a team April 12, 2021 19:58
@scottybollinger scottybollinger changed the title [Enterprise Search] Design Pass: role mappings [Enterprise Search] Design Pass: Role mappings Apr 12, 2021
@scottybollinger
re-align last cell to right
c82fd1b 
Accidentally deleted it refactoring
daveyholler
daveyholler approved these changes Apr 12, 2021
View reviewed changes
Copy link
Contributor

@daveyholler daveyholler left a comment

Choose a reason for hiding this comment

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

App Search is good by me. Thanks @scottybollinger

@johnbarrierwilson
Copy link
Member

For App Search's empty state on Users & Roles, can we remove the 1px border panel that's around the light gray/subdued panel? All else looks great!!

@scottybollinger
Copy link
Contributor Author

For App Search's empty state on Users & Roles, can we remove the 1px border panel that's around the light gray/subdued panel? All else looks great!!

That border seems consistent with every App Search view in Kibana and the separation in design was already in place. @daveyholler is that pattern something you want to keep?

@scottybollinger
Copy link
Contributor Author

If not, I assume you do still want to keep the border around the list of mappings, correct? Here's a mockup with that state:

2021-04-12_15-36-25 (1)

@scottybollinger
Conditionally have border for App Search
7d74576 
Requested to remove for empty state
@scottybollinger scottybollinger enabled auto-merge (squash) April 12, 2021 20:38
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
enterpriseSearch 1323 1332 +9

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
enterpriseSearch 2.0MB 2.0MB +2.6KB

History

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

yakhinvadim
yakhinvadim approved these changes Apr 13, 2021
View reviewed changes
Copy link
Contributor

@yakhinvadim yakhinvadim left a comment

Choose a reason for hiding this comment

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

I am also not sure if we want the disparity between AS and WS empty states.
But code changes LGTM.

@scottybollinger scottybollinger merged commit 93e270e into elastic:master Apr 13, 2021
@kibanamachine kibanamachine mentioned this pull request Apr 13, 2021
Merged
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Apr 13, 2021
@scottybollinger @kibanamachine
[Enterprise Search] Design Pass: Role mappings (elastic#96882)
30b7664 
* Update shared button color and panel shading

* Vertically align table cells to top

* [App Search] Update panels to have backgrounds not borders

* [Workplace Search] Update panels to have backgrounds not borders

* re-align last cell to right

Accidentally deleted it refactoring

* Conditionally have border for App Search

Requested to remove for empty state
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
✅ 7.x

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Apr 13, 2021
@kibanamachine @scottybollinger
[Enterprise Search] Design Pass: Role mappings (#96882) (#96962)
c3f609f 
* Update shared button color and panel shading

* Vertically align table cells to top

* [App Search] Update panels to have backgrounds not borders

* [Workplace Search] Update panels to have backgrounds not borders

* re-align last cell to right

Accidentally deleted it refactoring

* Conditionally have border for App Search

Requested to remove for empty state

Co-authored-by: Scotty Bollinger <scotty.bollinger@elastic.co>
@scottybollinger scottybollinger deleted the scottybollinger/7-13-polish-rolemappings branch June 24, 2021 20:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@daveyholler daveyholler daveyholler approved these changes

@yakhinvadim yakhinvadim yakhinvadim approved these changes

@johnbarrierwilson johnbarrierwilson Awaiting requested review from johnbarrierwilson

Assignees
No one assigned
Labels
auto-backport Deprecated - use backport:version if exact versions are needed Feature:Plugins release_note:skip Skip the PR/issue when compiling release notes v7.13.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@scottybollinger @johnbarrierwilson @kibanamachine @daveyholler @yakhinvadim