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.

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

Handle no option available case when local body returns empty response #7359

Merged
merged 4 commits into from
Apr 2, 2024

Conversation

mohanrajnambe
Copy link
Contributor

Proposed Changes

@coronasafe/care-fe-code-reviewers @coronasafe/code-reviewers

Merge Checklist

  • Add specs that demonstrate bug / test a new feature.
  • Update product documentation.
  • Ensure that UI text is kept in I18n files.
  • Prep screenshot or demo video for changelog entry, and attach it to issue.
  • Request for Peer Reviews
  • Completion of QA

@mohanrajnambe mohanrajnambe requested a review from a team as a code owner March 8, 2024 06:53
Copy link

vercel bot commented Mar 8, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
care-storybook ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 8, 2024 6:53am

Copy link

netlify bot commented Mar 8, 2024

Deploy Preview for care-egov-staging ready!

Name Link
🔨 Latest commit 80a9ef1
🔍 Latest deploy log https://app.netlify.com/sites/care-egov-staging/deploys/660852f99cdf1600073902a4
😎 Deploy Preview https://deploy-preview-7359--care-egov-staging.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Member

@rithviknishad rithviknishad left a comment

Choose a reason for hiding this comment

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

Wouldn't this be better if we moved this logic to the Select Menu component itself so it'd be applicable in all use cases?

@mohanrajnambe
Copy link
Contributor Author

That is a holistic approach. Let me update the PR accordingly!

Copy link

vercel bot commented Mar 14, 2024

@mohanrajnambe is attempting to deploy a commit to the Open Healthcare Network Team on Vercel.

A member of the Team first needs to authorize it.

@mohanrajnambe
Copy link
Contributor Author

mohanrajnambe commented Mar 14, 2024

Wouldn't this be better if we moved this logic to the Select Menu component itself so it'd be applicable in all use cases?

Since the logic of either disabling or the placeholder text 'No options available' relies on the length of the option, taking that into selectmenu component is not viable unless we bring in a new prop to configure that again. So, 'options.length' is a good to have in the component which is utilising the SelectFormField(SelectMenuV2)

LMK your thoughts

@rithviknishad
Copy link
Member

But options.length is accessible inside SelectMenu component too right, and since we need to make the behaviour same across the platform, it'd require changes to be made in all the references and not ideal.

@mohanrajnambe
Copy link
Contributor Author

mohanrajnambe commented Mar 15, 2024

I understand that we are trying to handle it in the root.

Validating the options.length for placeholder text will lead to display 'No options available' instead of 'Choose of an option' at the first render.

Here we bring data based on some other action. (for eg. based on what city is selected)

If we rely on options.length in the selectMenu itself, we loose the 'choose an option' placeholder initially and tend to display the 'no option available'. This is not the right way. Either we should handle it at where we use the component
or
Another approach to handle this is to move away from showing 'no option available' in the placeholder. Rather displaying an option when the selectmenu is clicked as 'no option available'(disabled thereby cannot select this as well).

We can disable the selectmenu based on option.length but the user will not know why it is disabled.

@rithviknishad
Copy link
Member

For the placeholder, simply show "Choose an option" if options.length > 0 else show "No options" and disable the component.

Copy link

Hi, This pr has been automatically marked as stale because it has not had any recent activity. It will be automatically closed if no further activity occurs for 7 more days. Thank you for your contributions.

@github-actions github-actions bot added stale merge conflict pull requests with merge conflict labels Mar 25, 2024
Copy link

👋 Hi, @mohanrajnambe,
Conflicts have been detected against the base branch. Please rebase your branch against the base branch.


This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there.

@github-actions github-actions bot removed the stale label Mar 28, 2024
@mohanrajnambe
Copy link
Contributor Author

image
When we have options loaded for State select, is displays the provided placeholder.
When the State select is not chosen, other dependent District select is disabled with No options placeholder

@rithviknishad

@mohanrajnambe
Copy link
Contributor Author

mohanrajnambe commented Mar 30, 2024

@rithviknishad
I have merged develop into this issue branch. But still the merge-conflicts label stays in this PR whereas when I compare it states that it can be merged
image

Any thoughts?

Copy link
Member

@rithviknishad rithviknishad left a comment

Choose a reason for hiding this comment

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

LGTM

@rithviknishad rithviknishad removed the merge conflict pull requests with merge conflict label Apr 1, 2024
@nihal467
Copy link
Member

nihal467 commented Apr 2, 2024

LGTM

@khavinshankar khavinshankar merged commit 2b71cd3 into ohcnetwork:develop Apr 2, 2024
33 of 35 checks passed
Copy link

github-actions bot commented Apr 2, 2024

@mohanrajnambe Your efforts have helped advance digital healthcare and TeleICU systems. 🚀 Thank you for taking the time out to make CARE better. We hope you continue to innovate and contribute; your impact is immense! 🙌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dropdown collapse when no data for local body
5 participants