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 dataset field level tags to UI #2729

Merged

Conversation

davidsharp7
Copy link
Member

@davidsharp7 davidsharp7 commented Jan 7, 2024

Problem

Currently the UI has no ability to add and delete tags at the field level. The proposed solution is update the DatasetTags component to allow for field level tagging/deletion and add this to the DatasetInfo component.

Closes: #2728

Solution

Screenshot 2024-02-09 at 07 07 16 Screenshot 2024-02-01 at 18 18 19

Update DatasetTags to include field level tagging in the UI.

One-line summary:

Update DatasetTags to include field level tagging in the UI

Checklist

  • You've signed-off your work
  • Your changes are accompanied by tests (if relevant)
  • Your change contains a small diff and is self-contained
  • You've updated any relevant documentation (if relevant)
  • You've included a one-line summary of your change for the CHANGELOG.md (Depending on the change, this may not be necessary).
  • You've versioned your .sql database schema migration according to Flyway's naming convention (if relevant)
  • You've included a header in any source code files (if relevant)

Signed-off-by: sharpd <number6labs@gmail.com>
@boring-cyborg boring-cyborg bot added the web label Jan 7, 2024
Copy link

netlify bot commented Jan 7, 2024

Deploy Preview for peppy-sprite-186812 canceled.

Name Link
🔨 Latest commit a54067e
🔍 Latest deploy log https://app.netlify.com/sites/peppy-sprite-186812/deploys/65c6dd7b1fa750000823e047

Copy link

codecov bot commented Jan 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (e93f951) 84.45% compared to head (a54067e) 84.45%.

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #2729   +/-   ##
=========================================
  Coverage     84.45%   84.45%           
  Complexity     1416     1416           
=========================================
  Files           251      251           
  Lines          6447     6447           
  Branches        291      291           
=========================================
  Hits           5445     5445           
  Misses          850      850           
  Partials        152      152           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@davidsharp7 davidsharp7 marked this pull request as ready for review January 7, 2024 18:24
Copy link
Member

@wslulciuc wslulciuc left a comment

Choose a reason for hiding this comment

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

LGTM 💯, but let's also get a +1 from @phixMe

@wslulciuc wslulciuc added this to the 0.45.0 milestone Jan 30, 2024
const handleOpen = (key: string) => {
setOpen(true)
setSelectedKey(key)
localStorage.setItem(`dsi_${dsNamespace}_${dsName}`, JSON.stringify(newExpandedRows))
Copy link
Member

Choose a reason for hiding this comment

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

I kind of want to get away from doing this because it doesn't move across urls. Could we possibly use use parameters for functionality like this?

</Collapse>
</TableCell>
</TableRow>
</React.Fragment>
Copy link
Member

Choose a reason for hiding this comment

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

From a UI perspective, I think it may be perhaps better to have tags that are already part of a dataset listed.

For new tags, I think a dialog would be pretty nice for this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Cool - thanks for the feedback. Yeah I agree it's not as slick as I would like. I have refactored and DatasetTags now looks like this:

Screenshot 2024-02-01 at 18 20 19 Screenshot 2024-02-01 at 18 20 48

sharpd added 2 commits February 1, 2024 18:14
Signed-off-by: sharpd <number6labs@gmail.com>
Signed-off-by: sharpd <number6labs@gmail.com>
Copy link
Member

@phixMe phixMe left a comment

Choose a reason for hiding this comment

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

Thanks @davidsharp7 This looks good now. We're going to have a merge conflict with the larger changes across the board, I may want to hold off merging this unless you need it immediately.

@davidsharp7
Copy link
Member Author

Thanks @davidsharp7 This looks good now. We're going to have a merge conflict with the larger changes across the board, I may want to hold off merging this unless you need it immediately.

Cool, cool - No giant rush from me @phixMe @wslulciuc .

@wslulciuc wslulciuc merged commit b27cb30 into MarquezProject:main Feb 10, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Add Dataset Field Tags to UI
3 participants