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

Fixing: Switch Blood Pressure and temperature dropdown to use TextFormField with type="number" instead of a dropdown #8644 #8728

Merged
merged 31 commits into from
Oct 16, 2024
Merged

Fixing: Switch Blood Pressure and temperature dropdown to use TextFormField with type="number" instead of a dropdown #8644 #8728

merged 31 commits into from
Oct 16, 2024

Conversation

AdityaJ2305
Copy link
Contributor

@AdityaJ2305 AdityaJ2305 commented Oct 6, 2024 •
edited
Loading

Proposed Changes

Status

  • Ready for review

Reviewers

@ohcnetwork/care-fe-code-reviewers
@rithviknishad
@nihal467

Screenshots

Screenshot 2024-10-07 at 12 16 35 AM Screenshot 2024-10-07 at 12 17 24 AM

Screen-Recording

Screen.Recording.2024-10-08.at.2.45.35.PM.1.mp4

Merge Checklist

  • Add specs that demonstrate bug / test a new feature.
  • 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

@netlify Netlify
Copy link

netlify bot commented Oct 6, 2024 •
edited
Loading

✅ Deploy Preview for care-ohc ready!

Name Link
🔨 Latest commit 72b0573
🔍 Latest deploy log https://app.netlify.com/sites/care-ohc/deploys/670fa75046cd4d0008f12686
😎 Deploy Preview https://deploy-preview-8728--care-ohc.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.

@AdityaJ2305 AdityaJ2305 marked this pull request as ready for review October 8, 2024 08:57
@AdityaJ2305 AdityaJ2305 requested a review from a team as a code owner October 8, 2024 08:57
@nihal467
Copy link
Member

nihal467 commented Oct 8, 2024

@AdityaJ2305 the cypress is failing in your PR , fix that as well

@AdityaJ2305
Copy link
Contributor Author

Hi @nihal467, All tests passed after my “cypress fail fix” commit. However, after merging the latest develop branch, the test for adding, duplicating, and deleting a bed is now failing.

@nihal467
Copy link
Member

nihal467 commented Oct 11, 2024 •
edited
Loading

image

  • restrict the temperature input field to accept only one decimal point, because all our existing data has only one decimal point
  • the c to f unit conversion in the temperature field is not working

@AdityaJ2305
Copy link
Contributor Author

image

  • restrict the temperature input field to accept only one decimal point, because all our existing data has only one decimal point
  • the c to f unit conversion in the temperature field is not working

Thank you for the feedback! I will proceed with the suggested changes. Just a quick clarification on the second checkpoint (Celsius to Fahrenheit): I’ve set it so that if the user selects C, they can only enter temperatures within the range specific to Celsius (35 to 41.1°C). Could you please elaborate on this requirement?

@nihal467
Copy link
Member

nihal467 commented Oct 11, 2024 •
edited
Loading

@AdityaJ2305 in the current staging version, if a user types 96.5 F, and when he/she clicks on the unit icon, this 96.5 F will automatically gets converted to the respective C value. This behavior occurs vice-versa

@AdityaJ2305
Copy link
Contributor Author

@AdityaJ2305 in the current staging version, if a user types 96.5 F, and when he/she clicks on the unit icon, this 96.5 F will automatically gets converted to the respective C value. This behavior occurs vice-versa

Thank you for clarifying! I’ll proceed with the changes accordingly.

rithviknishad
rithviknishad requested changes Oct 15, 2024
View reviewed changes
src/Locale/en.json Outdated Show resolved Hide resolved
rithviknishad
rithviknishad requested changes Oct 15, 2024
View reviewed changes
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.

mostly lgtm, just a minor type safety improvement

src/Components/Common/TemperatureFormField.tsx Outdated Show resolved Hide resolved
@github-actions github-actions bot added the Deploy-Failed Deplyment is not showing preview label Oct 16, 2024
@AdityaJ2305
Copy link
Contributor Author

AdityaJ2305 commented Oct 16, 2024 •
edited
Loading

@Nihal checks are failing after merging develop branch

@nihal467
Copy link
Member

LGTM

@nihal467 nihal467 added needs review tested and removed needs testing Deploy-Failed Deplyment is not showing preview labels Oct 16, 2024
@khavinshankar khavinshankar merged commit 1c1db13 into ohcnetwork:develop Oct 16, 2024
24 checks passed
@github-actions GitHub Actions
Copy link

@AdityaJ2305 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
Reviewers

@khavinshankar khavinshankar khavinshankar approved these changes

@rithviknishad rithviknishad rithviknishad approved these changes

Assignees
No one assigned
Labels
needs review tested
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Switch Blood Pressure and temperature dropdown to use TextFormField with type="number" instead of a dropdown
4 participants
@AdityaJ2305 @nihal467 @rithviknishad @khavinshankar