-
-
Notifications
You must be signed in to change notification settings - Fork 13
Tag module #2526
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
base: master
Are you sure you want to change the base?
Tag module #2526
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2526 +/- ##
=======================================
Coverage 77.99% 77.99%
=======================================
Files 281 281
Lines 20098 20098
Branches 2029 2029
=======================================
Hits 15676 15676
Misses 4376 4376
Partials 46 46
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
pylipp
left a comment
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.
Hey @MomoRazor , thanks a lot for this large PR! I could do some functional testing, and I'm already quite happy with the new module. I found some small bugs, and I have some UX questions that I'm getting Roanna's feedback for.
Bugs
- table breaks when filtering Application for All and Box
- in the selection for columns displayed in the table, the first (checkbox) column should not be listed; it's always displayed
- when creating/updating a tag, and selecting "Boxes" or "Beneficiaries" as Application, I can't submit the form. Maybe a zod issue?
UX questions @aerinsol please let us know your preferences:
- Tags overview - action buttons: side by side on the same level as the table like in dropapp, or unfolding from a menu? Are the icons ok?
- Tags overview column name "Application". In dropapp it is "Apply to". Change it or keep it?
- UpdateTag title: "Update Tag # 57" . It contains the ID. In UpdateBox we show "Box 12345678" as title. I think "Update Tag" would be fine
- created/lastModified info: in dropapp, the tag details page shows a box with created and last-modified info. We don't have this in v2 but we could add the created/lastModified columns in the TagsOverview (but hide them by default)
- UpdateTag: when changing the Application to "Boxes" or "Beneficiaries", it would be nice to have an info text appear (like from the
iinfo icon in dropapp): "changing type will unassign tags applied to boxes" or "...to beneficiaries" - CreateTag: would be nice to have a random color picked
| const { createToast } = useNotification(); | ||
| const { triggerError } = useErrorHandling(); | ||
|
|
||
| const { data: tagData, error: tagError } = useSuspenseQuery(TAG_QUERY, { |
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'm wondering if this query can be avoided since the data for the form could be taken from the row?
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.
Seeing as the Update Tag Form is a page right now, have you go any idea on how I can pass all the row information to a different page, rather then just pass the 'id' and pull the tag a new? Or should I consider the option of not having the update form in a completely separate link as it is now?
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.
Could you use the Apollo Cache maybe? When TagOverview is shown first, it should have all tags loaded by keys Tag:ID (I use the Apollo extension to view this in the browser dev tools).
If it's a cache miss for some reason (e.g. someone navigating directly to /tags/ID without having viewed TagOverview before), then the BE query should be run.
I haven't actually done this, it's an idea rather that would need a bit more investigation.
Re your question I'd keep the update form on a separate link.
|
@aerinsol some dropapp UX elements for reference: |
|
Thanks for the feedback @pylipp ! I'll probably get stuck in tomorrow :) |
|
Yeesh, I really don't like the interaction look for updating a tag. I know we use it for agreements and boxes but ugh. Doesn't seem to fit the tags - likely a good quick and dirty redesign task for Mariam. Nevertheless.... To answer your questions
Drop down is unnecessary here. I would keep it side by side so there's not an extra click.
Use the drop app name.
Agree.
Yes, would have this in the table. For the detailed view I would give it to Mariam.
Agree
Agree |
|
Hi guys! Quick update on this:
With those done, what remains are the following:
Let me know if I may have missed anything :) |
|
thanks for the update @MomoRazor |
Ah, I didn't realize you'd updated the screen cast! Screen cast is fine, just let me know next time. For the color picker it's more important that the user can input the html hex code for the color than the R/G/B channels, any chance you could adapt that? |
Screencast.from.2026-01-22.00-57-50.webm@aerinsol so the hex under the RGB Channel selector is a text field, and can be used to copy and paste hex into it. What I've also updated in the last push is the ability to that on the outside field as well, if you dismiss the colour selection window. This might make it easier to input in colours you've copied. Hopefully I've showed both ways you can input a hex colour directly with text in the above screan cap. |
|
LGTM! |
|
Quick update on this PR! So all the feedback should be handled and taken care of. The one thing I would like further feedback on specifically is the caching approach I've attempted on the individual Tag view. Essentially with the extra configurations I have passed to both the main Apollo Client, and the specific query, the individual view should be pulling from the main cache first, before pulling in new single tag information. To ensure that this is working well, I had implemented cache logging and I seemed to have gotten good hits and misses, however I'd love to know if the approach is good, as I depended quite a bit on Copilot to suppliment my (as of yet) limited knowledge of Apollo. That said, the only task I know I still have on this PR is to created some tests for the Tag Module. I'll be working on those next! |
|
I've added the tests for the Tag module (lots of AI help for this one!). Hope they have enough coverage! Let me know if they're up the scratch :). Should be all ready from my side |
thanks again @MomoRazor I'll check the test implementation this week. Currently 14 of the new tests fail in CI. Do they pass locally on your system? |
|
Hey @pylipp, sorry for the delay on this. I merged master and tried the tests again and they seem to be working locally. Maybe I'm running into the issue we have on the QR Scanner PR, if it is failing only on CI. If you think thats it, I'll be looking to switch to that pattern once I get the QR Scanner PR to work as requested |


This is the Tag Module PR, as I noticed I had not created this after having moved the Trello task to review, my bad! The following is a video of the current functionality:
Screencast.from.2026-01-08.18-55-28.webm