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

Fix layout of datamap page #2992

Conversation

allisonking
Copy link
Contributor

@allisonking allisonking commented Apr 4, 2023

Progress on #2976

Code Changes

  • Brings in the Layout component from the datamap UI as FixedLayout. It seemed like the conclusion was the only way to get the fixed/inside scrolling that the datamap UI has was to use a different layout component
  • Brought in more of the polish UI I missed from https://github.com/ethyca/fidesplus/pull/790

Steps to Confirm

  • Hopefully everything looks right now!!!

Pre-Merge Checklist

  • All CI Pipelines Succeeded
  • Documentation:
  • Issue Requirements are Met - no, I'll update the ticket since I didn't dedupe the PrivacyDeclaration components here. I think we can punt on that since I need to get back to privacy notice work this sprint
  • Relevant Follow-Up Issues Created
  • Update CHANGELOG.md
  • For API changes, the Postman collection has been updated

Description Of Changes

This branch should now have everything needed to run the datamap UI from admin UI. if anything looks different from the original datamap UI, do flag it!

@cypress
Copy link

cypress bot commented Apr 4, 2023

Passing run #1164 ↗︎

0 3 0 0 Flakiness 0

Details:

Merge ba35d05 into d02ecc4...
Project: fides Commit: ef00f16b27 ℹ️
Status: Passed Duration: 00:58 💡
Started: Apr 4, 2023 9:39 PM Ended: Apr 4, 2023 9:40 PM

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

@TheAndrewJackson TheAndrewJackson self-requested a review April 4, 2023 21:37
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.

This looks good to me! I think all of the layout issues I saw are fixed. I'm pretty sure all of the polish was transferred over. @NevilleS might catch something I missed though

@codecov
Copy link

codecov bot commented Apr 5, 2023

Codecov Report

❗ No coverage uploaded for pull request base (aking/2977/dezone-and-bring-nav-back@d02ecc4). Click here to learn what that means.
Patch has no changes to coverable lines.

❗ Current head ba35d05 differs from pull request most recent head 990f2dd. Consider uploading reports for the commit 990f2dd to get more accurate results

Additional details and impacted files
@@                           Coverage Diff                           @@
##             aking/2977/dezone-and-bring-nav-back    #2992   +/-   ##
=======================================================================
  Coverage                                        ?   86.86%           
=======================================================================
  Files                                           ?      303           
  Lines                                           ?    17177           
  Branches                                        ?     2195           
=======================================================================
  Hits                                            ?    14921           
  Misses                                          ?     1844           
  Partials                                        ?      412           

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 in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@allisonking allisonking marked this pull request as ready for review April 5, 2023 14:52
Copy link
Contributor

@NevilleS NevilleS left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @allisonking!

// Ideally we could have one common layout used by *all* pages, but we couldn't
// come up with a common method here without introducing other gotchas, so having
// a slightly different layout was decided as the most maintainable option
// (see https://github.com/ethyca/fidesplus/pull/709)
Copy link
Contributor

Choose a reason for hiding this comment

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

You can probably remove this link now, it'll be confusing in this repo

@@ -79,7 +79,7 @@ const TextInput = ({
setType(type === "password" ? "text" : "password");

return (
<InputGroup size="sm" mr="2">
<InputGroup size="sm">
Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome, thanks for porting this. There might be a wider impact on other forms than when I first did this work but I'm pretty confident it'll be an improvement...

@allisonking allisonking merged commit 5feec97 into aking/2977/dezone-and-bring-nav-back Apr 5, 2023
@allisonking allisonking deleted the aking/2976/merge-forked-components branch April 5, 2023 17:26
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.

3 participants