Skip to content
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.

Sign up for GitHub

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

Jump to bottom

'(fix)O3-2473': Visit Type selector fails and is not needed #1036

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

'(fix)O3-2473': Visit Type selector fails and is not needed #1036

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
31 changes: 17 additions & 14 deletions packages/esm-service-queues-app/src/patient-search/visit-form/base-visit-type.component.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,10 @@ interface BaseVisitTypeProps {
onChange: (event) => void;
patientUuid: string;
visitTypes: Array<VisitType>;
enableSearch?: boolean;
}

const BaseVisitType: React.FC<BaseVisitTypeProps> = ({ onChange, visitTypes }) => {
const BaseVisitType: React.FC<BaseVisitTypeProps> = ({ onChange, visitTypes, enableSearch = false }) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const { t } = useTranslation();
const isTablet = useLayoutType() === 'tablet';
const [searchTerm, setSearchTerm] = useState<string>('');
Expand All @@ -39,7 +40,7 @@ const BaseVisitType: React.FC<BaseVisitTypeProps> = ({ onChange, visitTypes }) =
[styles.tablet]: isTablet,
[styles.desktop]: !isTablet,
})}>
{results.length ? (
{enableSearch && (
<>
{isTablet ? (
<Layer>
Expand All @@ -56,19 +57,21 @@ const BaseVisitType: React.FC<BaseVisitTypeProps> = ({ onChange, visitTypes }) =
labelText=""
/>
)}

<RadioButtonGroup
className={styles.radioButtonGroup}
defaultSelected={defaultVisitType}
orientation="vertical"
onChange={onChange}
name="radio-button-group"
valueSelected={results?.length >= 1 && results[0].uuid}>
{results.map(({ uuid, display, name }) => (
<RadioButton key={uuid} className={styles.radioButton} id={name} labelText={display} value={uuid} />
))}
</RadioButtonGroup>
</>
)}

{results.length ? (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks right - moving the search conditional around the results table, but not around the actual search box itself.

<RadioButtonGroup
className={styles.radioButtonGroup}
defaultSelected={defaultVisitType}
orientation="vertical"
onChange={onChange}
name="radio-button-group"
valueSelected={results?.length >= 1 && results[0].uuid}>
{results.map(({ uuid, display, name }) => (
<RadioButton key={uuid} className={styles.radioButton} id={name} labelText={display} value={uuid} />
))}
</RadioButtonGroup>
) : (
<Layer>
<Tile className={styles.tile}>
Expand Down
27 changes: 24 additions & 3 deletions packages/esm-service-queues-app/src/patient-search/visit-form/base-visit-type.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,14 @@ jest.mock('@openmrs/esm-framework', () => ({

describe('BaseVisitType', () => {
it('renders visit types correctly', () => {
render(<BaseVisitType patientUuid={mockPatient.uuid} onChange={() => {}} visitTypes={mockVisitTypes} />);
render(
<BaseVisitType
patientUuid={mockPatient.uuid}
onChange={() => {}}
visitTypes={mockVisitTypes}
enableSearch={true}
/>,
);

const searchInput = screen.getByRole('searchbox');
expect(searchInput).toBeInTheDocument();
Expand All @@ -25,7 +32,14 @@ describe('BaseVisitType', () => {
it('handles search input correctly', async () => {
const user = userEvent.setup();

render(<BaseVisitType patientUuid={mockPatient.uuid} onChange={() => {}} visitTypes={mockVisitTypes} />);
render(
<BaseVisitType
patientUuid={mockPatient.uuid}
onChange={() => {}}
visitTypes={mockVisitTypes}
enableSearch={true}
/>,
);

const searchInput: HTMLInputElement = screen.getByRole('searchbox');
await user.type(searchInput, 'Visit Type 1');
Expand All @@ -37,7 +51,14 @@ describe('BaseVisitType', () => {
const user = userEvent.setup();

const mockOnChange = jest.fn();
render(<BaseVisitType patientUuid={mockPatient.uuid} onChange={mockOnChange} visitTypes={mockVisitTypes} />);
render(
<BaseVisitType
patientUuid={mockPatient.uuid}
onChange={() => {}}
visitTypes={mockVisitTypes}
enableSearch={true}
/>,
);

const radioButton: HTMLInputElement = screen.getByLabelText(mockVisitTypes[0].display);
await user.click(radioButton);
Expand Down