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

Added support for time in dateinput component #8504

Merged
merged 28 commits into from
Oct 28, 2024

Conversation

shivankacker
Copy link
Member

@shivankacker shivankacker commented Sep 8, 2024

Proposed Changes

image

@ohcnetwork/care-fe-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

@shivankacker shivankacker self-assigned this Sep 8, 2024
@shivankacker shivankacker requested a review from a team as a code owner September 8, 2024 21:56
Copy link

netlify bot commented Sep 8, 2024

Deploy Preview for care-ohc ready!

Name Link
🔨 Latest commit c3c1727
🔍 Latest deploy log https://app.netlify.com/sites/care-ohc/deploys/671f1a9afe17530008398d8d
😎 Deploy Preview https://deploy-preview-8504--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.

Copy link

cypress bot commented Sep 8, 2024

CARE    Run #3752

Run Properties:  status check passed Passed #3752  •  git commit c3c172789b: Added support for time in dateinput component
Project CARE
Branch Review support-time-in-dateinput
Run status status check passed Passed #3752
Run duration 03m 40s
Commit git commit c3c172789b: Added support for time in dateinput component
Committer Shivank Kacker
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 0
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 125
View all changes introduced in this branch ↗︎

@shivankacker shivankacker force-pushed the support-time-in-dateinput branch from d387e4d to e62e655 Compare September 12, 2024 09:48
@nihal467
Copy link
Member

nihal467 commented Sep 13, 2024

@shivankacker

image

  • When you open the date and time component in the patient consultation page, the entire form is scrolled to the right side, making it hard for the user to see what is the field heading since they are left-aligned
  • Once a user clicks on a specific time, then the component should auto-close

image

  • the placeholder text { hh: mm a }, what is meant by "a" in this text?

image

  • We had an existing behavior where typing "12092024" would automatically format the input as 12/09/2024. This behavior was removed in your PR. Please revert the change and retain the auto-formatting in the component.
  • When the user types "12092024" and moves out of focus of the element or switches tabs, the entered data resets to the current date and time in the discharge pop-up.

image

  • Since the auto-formatting is not working, there is no limit in user input
  • On mobile view, the component is clipping in, making it properly responsive across devices

image

  • Even if the user manually, types all the details by manual formatting, the component still reset to the current date and time

Personal Note: Bro, instead of modifying the entire Cypress test to make it pass, try to understand the core problem of test fail. Then, debug it

CC: @gigincg

@nihal467
Copy link
Member

nihal467 commented Sep 13, 2024

@shivankacker Bro, create a checklist of all the forms where you're changing the component and add it to the description. Review the functionality and responsiveness of each form one by one before requesting the next QA review.

@shivankacker
Copy link
Member Author

shivankacker commented Sep 13, 2024

@nihal467

  1. Will see to the scroll issue. Wasn't happening with my screen size. I even went ahead and fixed the overflow issue that the existing component was causing. The scroll resets once the time is entered.

Before:
WhatsApp Image 2024-09-13 at 21 56 42

After:
WhatsApp Image 2024-09-13 at 21 56 41

Will check and fix this one.

  1. I did not keep the auto closing thing because : It will close if a user will set the time before setting the date or vice-versa which makes it a UX issue.
  2. "a" means AM/PM. If it is not understandable I will change it to "am/pm"

We had an existing behavior where typing "12092024" would automatically format the input as 12/09/2024

  1. The existing behaviour made it easy to enter the date, but totally hard to update it. The reason it was removed was so that the user does not face this while editing the date:
Screen.Recording.2024-09-13.at.9.31.58.PM.mov

Because this behaviour was changed, the tests were also changed. Adding any format restrictions WHILE a user is typing would always result in these kind of issues.

  1. The input will auto reset if the entered time is not of the desired format.
  2. On mobile devices, the component can be scrolled left-right. This is because the date input would become small if i try to change sizes/ do anything else.
  3. You did not add space, which made it an invalid format.

cc. @gigincg

@shivankacker
Copy link
Member Author

shivankacker commented Sep 13, 2024

@nihal467

  • because you did not like the left-right scroll, I changed it to go the next row. Let me know if it doesn't look good.
  • changed "a" to "am/pm"
  • fixed the edge case of the overflow. Also verified all the inputs again. Please let me know if I missed out on something.
  • Also the input will show a red border now if the input format is wrong. Just an add on to make it fool proof.

cc. @gigincg

@github-actions github-actions bot added the Deploy-Failed Deplyment is not showing preview label Oct 24, 2024
@shivankacker shivankacker removed the Deploy-Failed Deplyment is not showing preview label Oct 24, 2024
Copy link
Member

@bodhish bodhish left a comment

Choose a reason for hiding this comment

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

Only covered 30% @shivankacker self review your code once.

package-lock.json Outdated Show resolved Hide resolved
src/components/Common/DateInputV2.tsx Outdated Show resolved Hide resolved
src/components/Common/DateInputV2.tsx Outdated Show resolved Hide resolved
src/components/Common/DateInputV2.tsx Outdated Show resolved Hide resolved
@github-actions github-actions bot added the Deploy-Failed Deplyment is not showing preview label Oct 25, 2024
@shivankacker shivankacker force-pushed the support-time-in-dateinput branch from f42f8ef to 880447e Compare October 25, 2024 04:28
@shivankacker shivankacker force-pushed the support-time-in-dateinput branch from 880447e to d5f0683 Compare October 25, 2024 04:31
@shivankacker shivankacker removed the Deploy-Failed Deplyment is not showing preview label Oct 25, 2024
@shivankacker shivankacker requested a review from bodhish October 25, 2024 04:40
@shivankacker
Copy link
Member Author

@bodhish cleaned up as much as possible

@github-actions github-actions bot added the merge conflict pull requests with merge conflict label Oct 27, 2024
Copy link

👋 Hi, @shivankacker,
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.

Copy link
Member

@bodhish bodhish left a comment

Choose a reason for hiding this comment

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

Lets merge this and see what breaks!

@bodhish
Copy link
Member

bodhish commented Oct 27, 2024

@shivankacker do fix the conflicts and merge it asap.

@github-actions github-actions bot removed the merge conflict pull requests with merge conflict label Oct 28, 2024
@shivankacker
Copy link
Member Author

@bodhish can you merge before another merge conflict happens 🥲

@rithviknishad rithviknishad requested review from nihal467 and removed request for khavinshankar and nihal467 October 28, 2024 07:24
@rithviknishad rithviknishad merged commit 8eb4dc4 into develop Oct 28, 2024
26 checks passed
@rithviknishad rithviknishad deleted the support-time-in-dateinput branch October 28, 2024 07:26
Copy link

@shivankacker @shivankacker 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! 🙌

@shivankacker
Copy link
Member Author

shivankacker commented Oct 28, 2024

image

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.

Care Date Time Component
4 participants