'(fix)O3-2473': Visit Type selector fails and is not needed #1036
Conversation
|
@mseaton, @mogoodrich, @ojwanganto, @makombe kindly help me review my PR at your convinient time please! |
|
Also added Chi Bong, since he is working a lot on queues now |
| } | ||
|
|
||
| const BaseVisitType: React.FC<BaseVisitTypeProps> = ({ onChange, visitTypes }) => { | ||
| const BaseVisitType: React.FC<BaseVisitTypeProps> = ({ onChange, visitTypes, enableSearch = false }) => { |
There was a problem hiding this comment.
I think the enableSearch is still a bit of a question mark. Personally, I see no reason to have a search here. But let's just leave the search as-is for now and we can assess more in the future.
There was a problem hiding this comment.
@mseaton Kindly could you please provide more insight into the reasons behind this opinion? Let's maintain the search feature as it is for now, and we can reassess its relevance and effectiveness in the future? I am a bit confused on the changes to be made basing on the review.
There was a problem hiding this comment.
@jwnasambu - issue with the search getting hidden is a real bug. The issue with the search not being necessary is an opinion I am expressing, but not really a problem. Given that the designs have the search, we can leave it for now and maybe start a conversation on the ux-design-advisory asking about the thinking behind the visit type search. It would be good to make sure we fully understand what use case this is envisioned for (eg. are there implementations out there that have so many visit types we would need an auto-complete search for them; who/what implementation is this designed for where this is a need?
There was a problem hiding this comment.
Beyond this, if we are going to have this boolean enableSearch prop on the component, then we will need to also support this configurability in the config schema and ensure the configured value is getting passed in and used when this component is instantiated. I don't see that in this PR, and it's not really priority for us to have this removed, so this is why I said just to leave it.
| </> | ||
| )} | ||
|
|
||
| {results.length ? ( |
There was a problem hiding this comment.
This looks right - moving the search conditional around the results table, but not around the actual search box itself.
Requirements
Summary
I modify the code to remove the search by default and allow it to be toggled back on with a configuration property by following these steps:
By doing these, when you use , by default, the search functionality will be disabled and If you want to enable it, you can set enableSearch to true.
Screenshots
screencast.2024-03-14.4.PM-42-31.mp4
Related Issue
https://openmrs.atlassian.net/browse/O3-2473
Other