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

[2290] Request details Approve & Deny button group #2473

Merged
merged 11 commits into from
Feb 6, 2023

Conversation

ssangervasi
Copy link
Contributor

@ssangervasi ssangervasi commented Feb 1, 2023

Closes #2290

Code Changes

  • Request details Approve & Deny button group

    • ApproveButton component
    • DenyButton component
    • Tests for approve & deny buttons
  • General improvements:

    • StyledButton for shared button styles
    • Use "on" for props and "handle" for handler functions
    • Fix returnFocusOnClose
    • Initial tests for requests table and details pages
    • getByTestIdPrefix Cypress command

Steps to Confirm

  • Details page buttons
    • Approve a request
    • Deny a request with reason modal
  • Table menu buttons
    • Should be verified as well because the components were refactored
    • Approve a request
    • Deny a request with reason modal

Pre-Merge Checklist

Description Of Changes

Updated video after resolving #2443 merge conflicts:

details-approve-conf.mp4

With the buttons refactored out, they can be dropped right into the page:
details-button

Video of using Deny button & modal:

details-deny.mp4

@ssangervasi ssangervasi force-pushed the ssangervasi/fides/2290/request-details-page-buttons branch 2 times, most recently from 8eb6668 to aafda14 Compare February 1, 2023 22:22
@codecov
Copy link

codecov bot commented Feb 1, 2023

Codecov Report

Base: 88.45% // Head: 88.45% // No change to project coverage 👍

Coverage data is based on head (db1e47c) compared to base (6f88689).
Patch has no changes to coverable lines.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2473   +/-   ##
=======================================
  Coverage   88.45%   88.45%           
=======================================
  Files         327      327           
  Lines       15957    15957           
  Branches     4431     4431           
=======================================
  Hits        14115    14115           
  Misses       1688     1688           
  Partials      154      154           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@ssangervasi
Copy link
Contributor Author

Still resolving the merge conflicts from #2443 which adds a confirmation for the approve button.

@ssangervasi ssangervasi force-pushed the ssangervasi/fides/2290/request-details-page-buttons branch from e750658 to 58d7252 Compare February 2, 2023 01:13
Copy link
Contributor

@TheAndrewJackson TheAndrewJackson left a comment

Choose a reason for hiding this comment

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

Thanks for refactoring this and adding the tests! Just one small nit. Otherwise everything looks great.

In my testing I noticed that the approval modal was styled slightly different between the table and details pages. Did you see this?

table:

Screenshot 2023-02-02 at 16 09 29

details:

Screenshot 2023-02-02 at 16 09 49

@ssangervasi ssangervasi force-pushed the ssangervasi/fides/2290/request-details-page-buttons branch from 2e2a02d to db1e47c Compare February 3, 2023 00:07
@ssangervasi
Copy link
Contributor Author

In my testing I noticed that the approval modal was styled slightly different between the table and details pages. Did you see this?

Looks like applying the size to the ButtonGroup made the style leak through to the modal buttons - I guess because the react context transcends portals 🌌.

Copy link
Contributor

@TheAndrewJackson TheAndrewJackson left a comment

Choose a reason for hiding this comment

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

I checked again and the styles are working now 👍 Thanks for the edits! :shipit:

@ssangervasi ssangervasi merged commit 64a4cc3 into main Feb 6, 2023
@ssangervasi ssangervasi deleted the ssangervasi/fides/2290/request-details-page-buttons branch February 6, 2023 19:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add "Approve/Deny Request" button on the Request Details page to be consistent with Privacy Requests page
2 participants