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

Add system redesign #2490

Merged
merged 8 commits into from
Feb 7, 2023
Merged

Add system redesign #2490

merged 8 commits into from
Feb 7, 2023

Conversation

allisonking
Copy link
Contributor

@allisonking allisonking commented Feb 2, 2023

Part of #2434

(probably should've made it a separate ticket, but the new splash screen snuck in after sprint planning!)

Based off of this design: https://www.figma.com/file/dmEwdK3xZwjKfGVQ9t66xe/Fides-v.2-Master-Working-Files?node-id=3116%3A111644&t=tKcOdaMgM7kn2hRS-0

Code Changes

  • Updates the add system page to match new design

Steps to Confirm

  • Visit /add-systems and make sure everything works ok!
  • Also try adjusting the width of your browser

Pre-Merge Checklist

Description Of Changes

Screen.Recording.2023-02-02.at.5.29.04.PM.mov

When the data flow scanner is disabled (i.e. unhealthy cluster)
image

note @rsilvery I kind of went rogue and plopped the little green indicator in here. it's not in the new designs, but it seemed useful to keep from the old implementation so that we can tell when the data scanner is available. this feature was never really fully designed, so happy to update it if we get a new design for it

Without the plus server:
image

With the side nav (which only appears once there are systems):
image

@codecov
Copy link

codecov bot commented Feb 2, 2023

Codecov Report

Base: 88.44% // Head: 88.43% // Decreases project coverage by -0.01% ⚠️

Coverage data is based on head (cbef8f5) compared to base (2205677).
Patch has no changes to coverable lines.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2490      +/-   ##
==========================================
- Coverage   88.44%   88.43%   -0.01%     
==========================================
  Files         328      328              
  Lines       15954    15954              
  Branches     4431     4431              
==========================================
- Hits        14110    14109       -1     
- Misses       1689     1690       +1     
  Partials      155      155              
Impacted Files Coverage Δ
src/fides/api/main.py 86.09% <0.00%> (-0.67%) ⬇️

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.

@rsilvery
Copy link
Contributor

rsilvery commented Feb 3, 2023

@allisonking looks good but just to make sure we're on the same page: Add Systems will go to the new screen with a list of systems next, right?

@allisonking
Copy link
Contributor Author

@allisonking looks good but just to make sure we're on the same page: Add Systems will go to the new screen with a list of systems next, right?

yup yup! that'll be the bulk of #2434

It does not need to be a form.

Also moves components around to start to match updated design
@allisonking allisonking force-pushed the aking/2434/add-system-redesign branch from aa6861a to 914b08a Compare February 3, 2023 19:29
@allisonking allisonking marked this pull request as ready for review February 6, 2023 16:01
@TheAndrewJackson
Copy link
Contributor

Reviewing now 👀

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.

The changes worked as expected. Only one small nit. I did notice one difference between the screenshots/video and my testing. The stepper was showing up.

I'm pretty sure we want the stepper. Do you have any insight into what makes the stepper show/not show? If the stepper showing is fine then it should be good to go!

Screenshot 2023-02-06 at 17 28 43

@rsilvery
Copy link
Contributor

rsilvery commented Feb 7, 2023

@allisonking, @TheAndrewJackson we do not want the stepper.

@allisonking
Copy link
Contributor Author

@allisonking, @TheAndrewJackson we do not want the stepper.

mm yeah the stepper shows up by default in dev environments because of the way our feature flags work. @rsilvery if we are sure we do not want the stepper anymore, can we remove it entirely from the code instead of it being feature flagged off?

@rsilvery
Copy link
Contributor

rsilvery commented Feb 7, 2023

@allisonking, @TheAndrewJackson we do not want the stepper.

mm yeah the stepper shows up by default in dev environments because of the way our feature flags work. @rsilvery if we are sure we do not want the stepper anymore, can we remove it entirely from the code instead of it being feature flagged off?

Yes. please!

@allisonking allisonking merged commit 5d0fa65 into main Feb 7, 2023
@allisonking allisonking deleted the aking/2434/add-system-redesign branch February 7, 2023 20:40
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