-
Notifications
You must be signed in to change notification settings - Fork 41
Ensure Correct COT Selection in QB When Sharing Formatters #6252
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
Conversation
Triggered by fed7786 on branch refs/heads/issue-5503
Triggered by db695a6 on branch refs/heads/issue-5503
specifyweb/frontend/js_src/lib/components/QueryBuilder/Formatter.tsx
Outdated
Show resolved
Hide resolved
specifyweb/frontend/js_src/lib/components/QueryBuilder/Formatter.tsx
Outdated
Show resolved
Hide resolved
sharadsw
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.
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! |
sharadsw
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.
Looks good, nice work!
specifyweb/frontend/js_src/lib/components/QueryBuilder/Formatter.tsx
Outdated
Show resolved
Hide resolved
…er.tsx Co-authored-by: Sharad S <16229739+sharadsw@users.noreply.github.com>
lexiclevenger
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.
- 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Verify that it selects the proper COT regardless of the Catalog Number format
Works as expected!
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.
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.
specifyweb/frontend/js_src/lib/components/QueryBuilder/Formatter.tsx
Outdated
Show resolved
Hide resolved
|
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 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
pashiav
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.
- Verify that it selects the proper COT regardless of the Catalog Number format=
Looks good!
@emenslin I didn't run into what you mentioned, it's working on my end!
melton-jason
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.
Nice work!
Coming along nicely 👌
Fixes #5503
Checklist
self-explanatory (or properly documented)
Testing instructions
AnytoEqual