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

Data Flow Modal #3008

Merged
merged 33 commits into from
Apr 12, 2023
Merged

Data Flow Modal #3008

merged 33 commits into from
Apr 12, 2023

Conversation

TheAndrewJackson
Copy link
Contributor

@TheAndrewJackson TheAndrewJackson commented Apr 7, 2023

Closes #2941

Code Changes

  • Added data flow modal and accordion
  • Updated links to build from ingress/egress fields
  • Added initial tests for accordion

Steps to Confirm

  • npm run dev from admin ui. nox -s "dev(fidesplus)" from fidesplus
  • Run a dataflow scan
  • Go to system config page. Navigate to data flow tab
  • Try deleting, adding, adding all, and removing all systems from both the source/destination accordion items
  • Go to datamap. Click on system node (or row in table)
  • Repeat all of testing steps.
  • The map should update the links as you make edits. NOTE: The graph looks really zoooooomed out right now due to having so many links. That will need to be addressed in a follow up

Pre-Merge Checklist

Description Of Changes

The initial code came from the user manage page that handles assigning roles. I tried to reuse it and make it modular but these flows are different enough that it started to become more complicated to make it usable for both. If this pattern starts to become more common it'll probably worth taking a step back and modularizing all of these components.

demo-of-data-flow.mov

The flow should be the same from the datamap

@cypress
Copy link

cypress bot commented Apr 7, 2023

Passing run #1281 ↗︎

0 3 0 0 Flakiness 0
⚠️ You've recorded test results over your free plan limit.
Upgrade your plan to view test results.

Details:

Merge 78448d9 into 2aa7858...
Project: fides Commit: fee086cf8e ℹ️
Status: Passed Duration: 00:34 💡
Started: Apr 11, 2023 8:36 PM Ended: Apr 11, 2023 8:36 PM

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

@TheAndrewJackson TheAndrewJackson marked this pull request as ready for review April 7, 2023 18:36
@TheAndrewJackson TheAndrewJackson self-assigned this Apr 7, 2023
@allisonking
Copy link
Contributor

Still testing things out, but going to report this one first. I'm having trouble navigating away from the Data Flow tab. I haven't looked into it, but if I had to guess, I'd point at the dirty form slice?

Screen.Recording.2023-04-10.at.3.21.37.PM.mov

@allisonking
Copy link
Contributor

Another thing—I think this flow should let me see a line on the spatial map for pixie operator, right?

Screen.Recording.2023-04-10.at.3.41.40.PM.mov

@allisonking
Copy link
Contributor

Oh never mind, I think the line is there, it just doesn't show in search since the destination node doesn't have pixie-operator as a source. tricky problem!

Copy link
Contributor

@allisonking allisonking left a comment

Choose a reason for hiding this comment

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

this is a really great improvement! I peppered some thoughts throughout. the main thing is the bug about not being able to tab away from the Data flow tab. I think we should work through what's going on with all the "new" links on the map, too (at least, I haven't wrapped my head around it!). Let me know if you want to pair!

and yes, I didn't realize this modal/assigning thing would be a pattern throughout the app. we should standardize it at some point, as you suggested.

@TheAndrewJackson
Copy link
Contributor Author

@allisonking Everything should be fixed now 👍

Copy link
Contributor

@allisonking allisonking left a comment

Choose a reason for hiding this comment

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

things are working well! left a few small things, but it's not blocking

@TheAndrewJackson TheAndrewJackson merged commit 41ae349 into main Apr 12, 2023
@TheAndrewJackson TheAndrewJackson deleted the ajackson_data_flow_modal branch April 12, 2023 14:17
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 data flow accordion and modal
2 participants