Ensure Correct COT Selection in QB When Sharing Formatters#6252
Conversation
Triggered by fed7786 on branch refs/heads/issue-5503
Triggered by db695a6 on branch refs/heads/issue-5503
sharadsw
left a comment
There was a problem hiding this comment.
I like your approach. Functionally your code looks good, just pointing out some minor code smells.
Triggered by a9e9de4 on branch refs/heads/issue-5503
Good point, I've added some helpers to separate the logic and hopefully make it a little more intuitive. Let me know if you think this looks better! |
…er.tsx Co-authored-by: Sharad S <16229739+sharadsw@users.noreply.github.com>
lexiclevenger
left a comment
There was a problem hiding this comment.
- Verify that it selects the proper COT regardless of the Catalog Number format
Looks good!
Screen.Recording.2025-02-24.at.11.42.46.AM.mov
pashiav
left a comment
There was a problem hiding this comment.
- Verify that it selects the proper COT regardless of the Catalog Number format
Works as expected!
There was a problem hiding this comment.
Nice work on this! I really like the implementation; pretty clever idea! 🚀
I will mention that while the CollectionObjectType the user selects while the page is loaded will always be respected, it will always revert to the first CollectionObjectType in the list with the same format when re-rendered!
Screen.Recording.2025-02-26.at.9.04.25.PM.mov
This is because only the format name (nothing of the CollectionObjectType) is saved to the SpQueryField in the database; when the page is rendered initially, it isn't formatted with the formatterSeparator and index and is just the name of the formatter itself.
So Specify has no clue which CollectionObjectType it can be referring to.
There's also a somewhat smaller Issue where changing the CollectionObjectType to another which shares the same format doesn't trigger a change on the field (because nothing has actually changed on the SpQueryField!)
Screen.Recording.2025-02-26.at.9.40.58.PM.mov
We're relatively having a lot of Issues regarding this feature (#6239, #6253, etc. no thanks to my initial implementation in the first place!), and I think that's usually an indicator to take a step back and re-evaluate the core problem and potential solutions.
@specify/ux-testing, @specify/dev-testing
Instead of having a separate option for each CollectionObjectType, could we instead have an option for each Format and have the title of the option be a combination of all the COT names with that format?
It could look something like this:
Or we can go even more descriptive to try and be as clear as possible: the user isn't selecting/filtering on COT, they're only applying a different formatter for the field:
Alternatively, we could move away from the SpQueryField -> FormatName approach entirely and come up with something different which would allow the user to select an individual COT name.
|
I agree with Jasons comment and I think that grouping them together might make the most sense since it does not necessarily matter which specific one you use. I also think since we don't really have the gear other places aside from when you have something mapped as 'formatter' or 'aggregator' that adding 'Format As: ...' in this instance makes sense but we can go with something else if people have other opinions @specify/ux-testing |
Triggered by 65009e7 on branch refs/heads/issue-5503
|
Great suggestions! I've updated the logic to group COTs with identical formatters together. This should mitigate issues related to re-rendering and state changes in the form, since it no longer compares against the same formatter. Also, I've added the "Format As:" identifier to improve clarity, but we can adjust it if it seems too confusing. |
emenslin
left a comment
There was a problem hiding this comment.
- Verify that it selects the proper COT regardless of the Catalog Number format
Looks good! I ran into an issue where the query results were incorrect (I set the filter to equal and instead it showed all results), I couldn't recreate it but I would love if we could get one more testing review to verify that it is not an issue
melton-jason
left a comment
There was a problem hiding this comment.
Nice work!
Coming along nicely 👌
Fixes #5503
Checklist
self-explanatory (or properly documented)
Testing instructions
AnytoEqual