-
Notifications
You must be signed in to change notification settings - Fork 327
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
Update dataset/dataset field tagging experience #2761
Update dataset/dataset field tagging experience #2761
Conversation
Signed-off-by: sharpd <davidsharp7@gmail.com>
Signed-off-by: sharpd <davidsharp7@gmail.com>
Signed-off-by: sharpd <davidsharp7@gmail.com>
✅ Deploy Preview for peppy-sprite-186812 canceled.
|
Signed-off-by: sharpd <davidsharp7@gmail.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2761 +/- ##
=========================================
Coverage 84.47% 84.47%
Complexity 1429 1429
=========================================
Files 251 251
Lines 6460 6460
Branches 299 299
=========================================
Hits 5457 5457
Misses 850 850
Partials 153 153 ☔ View full report in Codecov by Sentry. |
Signed-off-by: sharpd <davidsharp7@gmail.com>
web/src/store/reducers/datasets.ts
Outdated
@@ -32,7 +32,7 @@ export type IDatasetsState = { | |||
totalCount: number | |||
init: boolean | |||
deletedDatasetName: string | |||
refreshTags: boolean | |||
refreshTags: string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would this be a string and not a boolean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the thinking is twofold - one to bring it into line with
}, [lineageDataset.name, datasets.refreshTags]) |
where we are refreshing based on the change in dataset name.
Second reason was for trouble shooting purposes. Based on the concatanation of namespace/datasetname/field/tag it was just easier to understand/diagnose what was happening.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can always change the dependency to
datasets.refreshTags === true
and change the reducer to set refreshTags to False when its updating then True on success.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, let's just go with what you have here.
Signed-off-by: sharpd <davidsharp7@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @davidsharp7
Signed-off-by: sharpd <davidsharp7@gmail.com>
…rquez into bug/fix_tag_state
Signed-off-by: sharpd <davidsharp7@gmail.com>
Signed-off-by: sharpd <davidsharp7@gmail.com>
Signed-off-by: sharpd <davidsharp7@gmail.com>
Signed-off-by: sharpd <davidsharp7@gmail.com>
Signed-off-by: sharpd <davidsharp7@gmail.com>
Signed-off-by: sharpd <davidsharp7@gmail.com>
Signed-off-by: sharpd <davidsharp7@gmail.com>
} | ||
|
||
const handleDelete = (deletedTag: string) => { | ||
const index = selectedTags.indexOf(deletedTag) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can do this a little more functionally like:
const newSelectedTags = selectedTags.filter((tag) => deletedTag !== tag);
setSelectedTags(newSelectedTags);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe let's make this change and I'll merge if that sounds good?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Surely can.
Signed-off-by: sharpd <davidsharp7@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤩
@@ -100,4 +100,38 @@ public void testApp_testDatasetTagFieldDelete() { | |||
// assert that only SENSITIVE remains | |||
assertThat(taggedDatasetFieldDelete.getFields().get(0).getTags()).containsExactly("SENSITIVE"); | |||
} | |||
|
|||
@Test | |||
public void testApp_testDatasetTagFieldConflict() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❤️
* add key down escape for dialogs Signed-off-by: sharpd <davidsharp7@gmail.com> * add truthy condition of refreshTags Signed-off-by: sharpd <davidsharp7@gmail.com> * fix freshTags state Signed-off-by: sharpd <davidsharp7@gmail.com> * fix failing test Signed-off-by: sharpd <davidsharp7@gmail.com> * add tag sort order and update conflict insert sql Signed-off-by: sharpd <davidsharp7@gmail.com> * add tag integration ON CONFLICT test Signed-off-by: sharpd <davidsharp7@gmail.com> * draft changes to tagging Signed-off-by: sharpd <davidsharp7@gmail.com> * add missing freesolo Signed-off-by: sharpd <davidsharp7@gmail.com> * add field tag toggle Signed-off-by: sharpd <davidsharp7@gmail.com> * add multi select/fix fresh Signed-off-by: sharpd <davidsharp7@gmail.com> * fix failing test Signed-off-by: sharpd <davidsharp7@gmail.com> * fix DOM underline error Signed-off-by: sharpd <davidsharp7@gmail.com> * remove selectOption for tag update Signed-off-by: sharpd <davidsharp7@gmail.com> * add auto highlight prop to aid user selection Signed-off-by: sharpd <davidsharp7@gmail.com> * change tag deletion updated based on feedback Signed-off-by: sharpd <davidsharp7@gmail.com> --------- Signed-off-by: sharpd <davidsharp7@gmail.com> Co-authored-by: Willy Lulciuc <willy@datakin.com>
Problem
The Adding and deleting of tags was not providing an intuitive and seemless experience.
Solution
Based on the feedback i've changed how the datasets details/tags/info components work by introducing a switch which allows field level tags to be exposed. Refresh only now occurs when the slide toggle is true. You can add multiple tags in one go now as well.
One-line summary:
Update dataset/dataset field tagging experience.
Checklist
CHANGELOG.md
(Depending on the change, this may not be necessary)..sql
database schema migration according to Flyway's naming convention (if relevant)