-
Notifications
You must be signed in to change notification settings - Fork 319
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 Job tagging to UI #2837
Add Job tagging to UI #2837
Conversation
Signed-off-by: sharpd <[email protected]>
✅ Deploy Preview for peppy-sprite-186812 canceled.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2837 +/- ##
=========================================
Coverage 84.74% 84.74%
Complexity 1456 1456
=========================================
Files 253 253
Lines 6562 6562
Branches 305 305
=========================================
Hits 5561 5561
Misses 850 850
Partials 151 151 ☔ View full report in Codecov by Sentry. |
Signed-off-by: sharpd <[email protected]>
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.
LGTM! I've added @phixMe as a reviewer to get this merged for our 0.48.0
release.
web/src/components/jobs/JobTags.tsx
Outdated
setTagDescription(selectedTagData ? selectedTagData.description : 'No Description') | ||
} | ||
|
||
const handleDescriptionChange = (event: any) => { |
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.
I think you can have an explicit type here, right?
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.
Ah yes good spot.
web/src/store/reducers/jobs.ts
Outdated
case ADD_JOB_TAG: | ||
return { | ||
...state, | ||
} | ||
case ADD_JOB_TAG_SUCCESS: | ||
return { | ||
...state, | ||
} | ||
case DELETE_JOB_TAG: | ||
return { | ||
...state, | ||
} | ||
case DELETE_JOB_TAG_SUCCESS: | ||
return { | ||
...state, | ||
} |
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.
Do we need any of these, if they just reflect the same state?
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.
Yip happy to remove.
web/src/components/jobs/JobTags.tsx
Outdated
const handleTagDescChange = (_event: any, value: string) => { | ||
const selectedTagData = tagData.find((tag) => tag.name === value) | ||
setListTag(value) | ||
setTagDescription(selectedTagData ? selectedTagData.description : 'No Description') |
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.
"No description" is kind of a description in and of itself, is this what you want out of this, would you prefer if it was just empty?
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.
Yeah that's my data hat talking - can leave blank.
web/src/components/jobs/JobTags.tsx
Outdated
<MQText subheading bottomMargin> | ||
Description | ||
</MQText> |
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.
I was thinking we may want to use the label
prop on these, maybe it would work a bit better with the design system. I could be wrong.
Signed-off-by: sharpd <[email protected]>
Signed-off-by: sharpd <[email protected]>
Signed-off-by: sharpd <[email protected]>
Signed-off-by: sharpd <[email protected]>
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 for driving this effort forward!
Always an opportunity to learn more stuff! |
Problem
Currently we can tag jobs via the API but not in UI.
Closes: #2817
Solution
Copy the basic infrastructure for tagging setting but apply it to the Job Detail page in the UI.
One-line summary:
Add job tagging to UI
Checklist
CHANGELOG.md
(Depending on the change, this may not be necessary)..sql
database schema migration according to Flyway's naming convention (if relevant)