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

Dataset UI Adjustments #2149

Merged
merged 16 commits into from
Jan 11, 2023
Merged

Conversation

SteveDMurphy
Copy link
Contributor

@SteveDMurphy SteveDMurphy commented Jan 5, 2023

Closes fidesplus#490

Code Changes (listed by commit)

For http://localhost:8080/dataset/new

  • Remove disabled button to manually generate a dataset
  • Update page text to "Create a dataset using YAML or connect to a database"
  • Change button text to Connect to a database
  • Modify database connection text to:
Connect to a database using the connection URL.
You may have received this URL from a colleague
or your Ethyca developer support engineer.
  • Simplify and update naming in tooltip for Classify dataset
  • Load the dataset on click instead of requiring the load dataset button
  • Update failing cypress tests
  • Remove unused toggling of selected row

Steps to Confirm

Pre-Merge Checklist

Description Of Changes

These are just some of the simpler changes, the remaining changes more aligned with classify may will end up split out to a separate PR for size reasons

Pushing this up and planning to ask for help around how to manage the state so the navigation back works as well. Additionally, the toast message loads twice. This commit attempts to move the `handleLoadDataset` function call to be done on page load if there is an `activeDatasetFidesKey` (essentially what happened on the button click) however I think I am getting lost in some of the state management with dispatch calls from DatasetTable.tsx
@SteveDMurphy
Copy link
Contributor Author

@allisonking or @TheAndrewJackson would love any insight or direction you may have on where I have gone wrong with removing the load dataset button in 8e025bb 😬

Tried to record my screen with it but essentially I was trying to move the handleLoadDataset function being called by the button click up to on load and although that works it creates a bunch of wonkiness in the state (loading the success message twice and breaking the navigation back) - thank you for any help!

Screen.Recording.2023-01-09.at.5.22.31.PM.mov

SteveDMurphy and others added 2 commits January 9, 2023 23:52
By breaking out the `handleLoadataset` functionality and moving it upstream into the `handleRowClick` we are able to remove the need for the load dataset button.

Co-authored-by: Allison King <[email protected]>
@SteveDMurphy SteveDMurphy self-assigned this Jan 10, 2023
@SteveDMurphy
Copy link
Contributor Author

SteveDMurphy commented Jan 10, 2023

@allisonking and @rsilvery here is a loom video of the changes as well -> https://www.loom.com/share/266cecc6f5f349e18472afd3d87a1f08

@SteveDMurphy SteveDMurphy marked this pull request as ready for review January 10, 2023 10:44
CHANGELOG.md Outdated Show resolved Hide resolved
clients/admin-ui/src/features/dataset/DatasetTable.tsx Outdated Show resolved Hide resolved
clients/admin-ui/src/features/dataset/DatasetTable.tsx Outdated Show resolved Hide resolved
clients/admin-ui/src/pages/dataset/index.tsx Show resolved Hide resolved
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.

nice job figuring out all the redux/cypress!! these are great improvements 💯

@SteveDMurphy SteveDMurphy merged commit 73465b7 into main Jan 11, 2023
@SteveDMurphy SteveDMurphy deleted the SteveDMurphy-fidesplus490-ux-changes branch January 11, 2023 16:10
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