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

Remove broken pagination and disable inputs on Inventory Management #4028

Merged
merged 2 commits into from
Dec 22, 2022

Conversation

Ashesh3
Copy link
Member

@Ashesh3 Ashesh3 commented Nov 10, 2022

Fixes #4027

Proposed Changes

  • While updating the Minimum Quantity, the Inventory Name is already not editable but not marked as disabled, giving a false sensation that input is allowed to be modified. This PR marks the field as disabled.
  • Removed broken pagination from Inventory management as it's not used nor needed since under any circumstances there can be at max 7 items in total, so no pagination is needed (and is not implemented).

image

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

@Ashesh3 Ashesh3 requested a review from a team November 10, 2022 15:06
@Ashesh3 Ashesh3 requested a review from a team as a code owner November 10, 2022 15:06
@Ashesh3 Ashesh3 requested a review from gigincg November 10, 2022 15:06
@vercel
Copy link

vercel bot commented Nov 10, 2022

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

Name Status Preview Updated
care-storybook ✅ Ready (Inspect) Visit Preview Dec 21, 2022 at 5:51AM (UTC)

@netlify
Copy link

netlify bot commented Nov 10, 2022

Deploy Preview for care-egov-staging ready!

Name Link
🔨 Latest commit 6fbb047
🔍 Latest deploy log https://app.netlify.com/sites/care-egov-staging/deploys/63a29e5e23bf2c00097d5bda
😎 Deploy Preview https://deploy-preview-4028--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 settings.

@nihal467
Copy link
Member

test approved

@@ -175,6 +174,7 @@ export const SetInventoryForm = (props: any) => {
type="string"
value={currentUnit}
errors=""
disabled={true}
Copy link
Member

Choose a reason for hiding this comment

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

If we are setting this as disabled always, why should this be an input field, can we design it better?

Copy link
Member Author

Choose a reason for hiding this comment

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

It conveys to me that this field was manually set (input) by a user and is no longer editable. More like, "This field is not available for editing". Do you recommend converting it to a plain text?

Copy link
Member Author

Choose a reason for hiding this comment

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

@khavinshankar any updates on this?

Copy link
Member

Choose a reason for hiding this comment

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

@Ashesh3 can we do something like a badge maybe
image

@nihal467 what's your take

Copy link
Member

@nihal467 nihal467 Dec 8, 2022

Choose a reason for hiding this comment

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

@khavinshankar Lets take it up in the EOD

@@ -149,6 +148,7 @@ export const UpdateMinQuantity = (props: any) => {
type="string"
value={data}
errors=""
disabled={true}
Copy link
Member

Choose a reason for hiding this comment

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

If we are setting this as disabled always, why should this be an input field, can we design it better?

@khavinshankar khavinshankar added the question Further information is requested label Nov 16, 2022
@github-actions github-actions bot added the merge conflict pull requests with merge conflict label Nov 17, 2022
@github-actions
Copy link

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

@Ashesh3 Ashesh3 removed question Further information is requested merge conflict pull requests with merge conflict labels Nov 22, 2022
@github-actions github-actions bot added the merge conflict pull requests with merge conflict label Nov 22, 2022
@github-actions
Copy link

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

@Ashesh3 Ashesh3 removed the merge conflict pull requests with merge conflict label Nov 22, 2022
@khavinshankar khavinshankar added the question Further information is requested label Nov 24, 2022
@github-actions
Copy link

github-actions bot commented Dec 1, 2022

👋 Hi, @Ashesh3,
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 added the merge conflict pull requests with merge conflict label Dec 1, 2022
@nihal467
Copy link
Member

nihal467 commented Dec 4, 2022

@Ashesh3 what is the update on this PR

@Ashesh3
Copy link
Member Author

Ashesh3 commented Dec 4, 2022

@Ashesh3 what is the update on this PR

Hey, @khavinshankar mentioned you in the review above and requested your take on what should be done here for inputs. If you could respond there and let me know, i can proceed accordingly.

@khavinshankar
Copy link
Member

@Ashesh3 what is the update on this PR

Hey, @khavinshankar mentioned you in the review above and requested your take on what should be done here for inputs. If you could respond there and let me know, i can proceed accordingly.

#4028 (comment)

@Ashesh3
Copy link
Member Author

Ashesh3 commented Dec 7, 2022

@nihal467 Any updates on this? Please check the conversation above.

@nihal467
Copy link
Member

@Ashesh3 @khavinshankar so what are we doing now of this PR

@nihal467 nihal467 added the waiting for update PR is been dead for a long time label Dec 13, 2022
@Ashesh3
Copy link
Member Author

Ashesh3 commented Dec 13, 2022

@Ashesh3 @khavinshankar so what are we doing now of this PR

Tried a few different designs, will showcase the modal design tomorrow.

@Ashesh3
Copy link
Member Author

Ashesh3 commented Dec 18, 2022

@nihal467 @khavinshankar Thoughts on the new design?

  • Replaced the page with a modal

TomatoIndianskimmer

@Ashesh3 Ashesh3 added needs testing and removed question Further information is requested tested waiting for update PR is been dead for a long time merge conflict pull requests with merge conflict Team Discuss Required labels Dec 18, 2022
@khavinshankar
Copy link
Member

Looks great, @nihal467 your thoughts?

@nihal467
Copy link
Member

@Ashesh3 Looks good to me, marking it for team discussion required for final approval on UI

@sonarcloud
Copy link

sonarcloud bot commented Dec 21, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 6 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@khavinshankar khavinshankar merged commit 5bc7992 into ohcnetwork:develop Dec 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Possible issue with Pagination
3 participants