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

[Look&Feel] Consistency and density improvements #2101

Merged
merged 12 commits into from
Aug 26, 2024

Conversation

danieldong51
Copy link
Contributor

@danieldong51 danieldong51 commented Aug 21, 2024

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

  • New functionality includes testing
  • New functionality has been documented
  • Commits are signed per the DCO using --signoff

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.

danieldong51 and others added 8 commits August 21, 2024 14:51
Signed-off-by: Dan Dong <danieldong51@gmail.com>
Signed-off-by: Dan Dong <danieldong51@gmail.com>
Signed-off-by: Dan Dong <danieldong51@gmail.com>
Signed-off-by: Dan Dong <danieldong51@gmail.com>
Signed-off-by: Dan Dong <danieldong51@gmail.com>
Signed-off-by: Dan Dong <danieldong51@gmail.com>
Signed-off-by: Dan Dong <danieldong51@gmail.com>
@derek-ho
Copy link
Collaborator

derek-ho commented Aug 22, 2024

@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

Signed-off-by: Dan Dong <danieldong51@gmail.com>
Copy link

codecov bot commented Aug 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files
@@           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           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Dan Dong <danieldong51@gmail.com>
@derek-ho
Copy link
Collaborator

@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
Copy link
Contributor Author

@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!

derek-ho
derek-ho previously approved these changes Aug 23, 2024
@derek-ho derek-ho added the backport 2.x backport to 2.x branch label Aug 23, 2024
Copy link
Member

@DarshitChanpura DarshitChanpura left a comment

Choose a reason for hiding this comment

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

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?

cwperks
cwperks previously approved these changes Aug 23, 2024
@danieldong51
Copy link
Contributor Author

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
Copy link

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
Copy link

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
Copy link
Contributor Author

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.

Signed-off-by: Dan Dong <danieldong51@gmail.com>
Signed-off-by: Dan Dong <danieldong51@gmail.com>
Copy link

@virajsanghvi virajsanghvi left a comment

Choose a reason for hiding this comment

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

Looks good to me assuming failing test is okay

@derek-ho derek-ho merged commit c143bce into opensearch-project:main Aug 26, 2024
18 of 19 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Aug 26, 2024
* Look&Feel Modal Changes

Signed-off-by: Dan Dong <danieldong51@gmail.com>

* Look&Feel EuiTabs Changes

Signed-off-by: Dan Dong <danieldong51@gmail.com>

* Look&Feel Semantic Headers and Paragraph Changes

Signed-off-by: Dan Dong <danieldong51@gmail.com>

* Look&Feel Context Menu Changes

Signed-off-by: Dan Dong <danieldong51@gmail.com>

* Look&Feel External Button Changes

Signed-off-by: Dan Dong <danieldong51@gmail.com>

* Look&Feel Navigational Button Changes

Signed-off-by: Dan Dong <danieldong51@gmail.com>

* Look&Feel Render Component Changes

Signed-off-by: Dan Dong <danieldong51@gmail.com>

* Fixed jest and cypress testing

Signed-off-by: Dan Dong <danieldong51@gmail.com>

* Fixed jest and cypress testing

Signed-off-by: Dan Dong <danieldong51@gmail.com>

* Revert button changes

Signed-off-by: Dan Dong <danieldong51@gmail.com>

* Updated jest testing

Signed-off-by: Dan Dong <danieldong51@gmail.com>

---------

Signed-off-by: Dan Dong <danieldong51@gmail.com>
Co-authored-by: Derek Ho <dxho@amazon.com>
(cherry picked from commit c143bce)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x backport to 2.x branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants