search

opensearch-project / security-dashboards-plugin

🔐 Manage your internal users, roles, access control, and audit logs from OpenSearch Dashboards
https://opensearch.org/docs/latest/security-plugin/index/
Apache License 2.0
71 stars 161 forks source link

[Look&Feel] Consistency and density improvements #2101

Closed danieldong51 closed 2 months ago

danieldong51 commented 3 months ago

Description

This PR applies the following Look & Feel density and consistency improvements:

  1. Use small context menus
  2. Standardize paragraph font size (15.75px next theme / 14px V7 theme)
  3. Use semantic headers (H1s on main pages, H2s on modals/flyouts)
  4. Using small tabs
  5. Use small padding on popovers

This PR, along with #2079, should mean all of the Look & Feel pattern guidances are applied to the Security Dashboards plugin.

Category

Enhancement

Why these changes are required?

UX

Screenshots

Context Menus

Removed popover padding from context menu Scope Before After
Tenant Actions Tenants Actions Before Tenants Actions Post

Paragraph Size

Scope Before After
Audit Log Audit Log Storage Before Audit Log Storage Post
Internal User Internal User Create Before Internal User Create Post
Roles: Duplicate Roles Duplicate Before Roles Duplicate Post
Roles: Map Roles Map User Before Roles Map User Post

Headers

Scope Before After
Audit Logs Audit Logs Before Audit Logs Post
Audit Logs: Settings Audit Logs Settings Before Audit Logs Settings Post
Authentication Authentication Before Authentication Post
Get Started Get Started Before Get Started Post
Internal User: Create Internal User Create Before Internal User Create Post
Internal User: List Internal User List Before Internal User List Post
Permission Permissions Before Permissions Post
Roles: Create Roles Create Before Roles Create Post
Roles: List Roles List Before Roles List Post
Roles: Map Roles Map User Before Roles Map User Post
Roles: Role Page Roles Role Before Roles Role Post
Tenancy Tenancy Before Tenancy Post

Modals

Scope Before After
Account: Reset Account Reset Password Before Account Reset Password Post
Account: Select Account Select Tenant Before Account Select Tenant Post
Account: View Account View Roles 1 Before Account View Roles 1 Post
Account: View Account View Roles Header Before Account View Roles Header Post
Configuration Configuration Create Before Configuration Create Post
Delete Delete Before Delete Post
Expression Expression Before Expression Post
Tenant: Default Tenant Default Before Tenant Default Post
Tenant: Edit Tenant Edit Before Tenant Edit Post
Tenant: Save Tenant Save 2 Before Tenant Save 2 Post

Tabs

Scope Before After
Role View Role View Before Role View Post
Tenants Tenants Before Tenants Post

Check List

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. For more information on following Developer Certificate of Origin and signing off your commits, please check here.

derek-ho commented 3 months ago

@danieldong51 can you do us a huge favor and address cypress failures on the workflows in this repo and in FTR? For example I see a following error originating from FTR (which I think has to do with changing the h3 tag of the title)

  1) Audit logs page
       should load Audit logs page properly:
     AssertionError: Timed out retrying after 60000ms: Expected to find content: 'Audit logging' within the selector: 'h3' but never did.
      at Context.eval (http://localhost:5601/__cypress/tests?p=cypress/integration/plugins/security/audit_log_spec.js:172:10)

Similarly, for your PRs in other repos, we may want to update these to make sure 2.17 release goes smoothly

codecov[bot] commented 3 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 71.39%. Comparing base (b1148fb) to head (3b37f3a). Report is 1 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #2101 +/- ## ======================================= Coverage 71.39% 71.39% ======================================= Files 97 97 Lines 2650 2650 Branches 410 403 -7 ======================================= Hits 1892 1892 Misses 642 642 Partials 116 116 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

derek-ho commented 3 months ago

@danieldong51 looks like the CI issues within this repo are fixed. Can we open a draft PR in the FTR and we can continue to address those/after the build succeeds? Once that PR is linked here I feel good about merging thanks!

danieldong51 commented 3 months ago

@danieldong51 looks like the CI issues within this repo are fixed. Can we open a draft PR in the FTR and we can continue to address those/after the build succeeds? Once that PR is linked here I feel good about merging thanks!

Yes, I just opened one here!

danieldong51 commented 3 months ago

Changes look good. Thank you @danieldong51 for taking time to upload screenshots. One question I have is to understand "Use primary buttons only for primary calls for action". Does it mean the design is reserved only for calls like Submit, Delete, etc. If yes, this change will make the buttons, without the boundary, look like href links. Is this expected?

I believe that buttons that are navigational should be turned into empty / text buttons, so that is expected. I am waiting on review from the Look&Feel team to make sure these buttons are navigational @virajsanghvi.

virajsanghvi commented 3 months ago

I would revert this one (going from primary action to just a secondary link seems like a big dropdoff). As its the cta of the workflow, I think keeping it as-is is safer

image

Similarly, can we keep these as is: I think they're the main CTA for these steps (Agree not ideal having multiple CTAs on same page, but I'd rather have a designer make a call here and not sure anyone has bandwidth):

image
virajsanghvi commented 3 months ago

Changes look good. Thank you @danieldong51 for taking time to upload screenshots. One question I have is to understand "Use primary buttons only for primary calls for action". Does it mean the design is reserved only for calls like Submit, Delete, etc. If yes, this change will make the buttons, without the boundary, look like href links. Is this expected?

I believe that buttons that are navigational should be turned into empty / text buttons, so that is expected. I am waiting on review from the Look&Feel team to make sure these buttons are navigational @virajsanghvi.

It is subjective. Ideally there's only a single primary button/cta on a page/surface. For purely navigational buttons, ones that aren't necessarily helping customers complete an action or save a form, the guidance is to use an empty button/link. But again, subjective, and as page owners, if you think any of these aren't the right treatment, it's fine to request no change here. You will be having reviews with UX and they can point out if any changes are necessary (they are stretched thin atm otherwise I'd just ask now).

danieldong51 commented 3 months ago

I would revert this one (going from primary action to just a secondary link seems like a big dropdoff). As its the cta of the workflow, I think keeping it as-is is safer image

Similarly, can we keep these as is: I think they're the main CTA for these steps (Agree not ideal having multiple CTAs on same page, but I'd rather have a designer make a call here and not sure anyone has bandwidth): image

Reverted external button component and primary button instances changes.