Fixes greyed out 'Active Manage Bed' button in Location Management Page #8814 #8827

abhip161 · 2024-10-18T13:54:13Z

Proposed Changes

Fixes Bug: Active Manage Bed button in Location Management Page is greyed out #8814
Change 1 Resolved issue where the 'Active Manage Bed' button was greyed out in the Location Management Page.

@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

netlify · 2024-10-18T13:54:33Z

✅ Deploy Preview for care-ohc ready!

Name	Link
🔨 Latest commit	`e3515fb`
🔍 Latest deploy log	https://app.netlify.com/sites/care-ohc/deploys/67188da2a2c63a0008927e50
😎 Deploy Preview	https://deploy-preview-8827--care-ohc.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

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

rithviknishad

isn't this the proper fix?

rithviknishad · 2024-10-18T15:43:18Z

src/Components/Facility/LocationManagement.tsx

@@ -289,7 +289,7 @@ const Location = ({
        border
        className={cn(
          "mt-3 flex w-full items-center justify-between",
-          totalBeds != null && "opacity-50",
+          totalBeds != null && "opacity-100",


Suggested change

totalBeds != null && "opacity-100",

totalBeds == null && "opacity-50",

hey @rithviknishad
if opacity-50 manage bed button will greyed out

if you want
opacity behaves correctly based on the totalBeds condition ??

When totalBeds is null, the button will have opacity-50, making it look greyed out.

When totalBeds has a value, the button will display with opacity-100, appearing fully visible.

I think we should disable the button, why hack the class? Set disabled: true?

abhip161 · 2024-10-21T06:22:35Z

Hey @bodhish ,
Could you please take a look at this asset?

Screen.Recording.2024-10-21.114442.mp4

Let me know if it can be modified or improved. Thanks!

rithviknishad · 2024-10-21T07:04:24Z

The button seems to be disabled if beds count is 0. How would I be able to add a bed for that location now?

null/undefined means data is still being fetched.
0 means no bed.

we should disable only for null/undefined right?

abhip161 · 2024-10-21T09:14:01Z

Hey @rithviknishad ,
I’ve updated the code to disable the button only when totalBeds is null or undefined,
.Please check it out!

rithviknishad · 2024-10-21T09:20:16Z

Now that it's disabled if null, we don't need to add conditional classes like @bodhish suggested right?

abhip161 · 2024-10-21T09:43:06Z

hey @rithviknishad ,
I see your point about simplifying the classes since the button is disabled when totalBeds is null. However, I think keeping the conditional classes for opacity is helpful for providing visual feedback. It allows users to see that the button is inactive (with reduced opacity) when there are no beds available, enhancing usability.

But I'm open to further discussion if you think a simpler approach would be better. Let me know your thoughts!

rithviknishad · 2024-10-21T09:45:19Z

The ButtonV2 component already applies necessary classes for it when disabled.

https://github.com/ohcnetwork/care_fe/blob/develop/src/Components/Common/components/ButtonV2.tsx#L101

abhip161 · 2024-10-21T10:03:33Z

hey @rithviknishad ,
Thanks for pointing that out! the ButtonV2 component already handles the necessary styles when it's disabled
I’ll update the code accordingly to rely on the built-in styles from ButtonV2. Thanks for your guidance!

…talBeds is null

…thub.com/abhip161/care_fe into issues/8814/fix-active-manage-bed-button

samholics · 2024-10-21T17:23:29Z

Tested working fine @nihal467

nihal467 · 2024-10-21T17:41:47Z

LGTM

src/Components/Facility/LocationManagement.tsx

bodhish · 2024-10-22T01:35:55Z

src/Components/Facility/LocationManagement.tsx

-        )}
-        href={`location/${id}/beds`}
+        className={cn("mt-3 flex w-full items-center justify-between")}
+        href={totalBeds != null ? `location/${id}/beds` : undefined}


Intresting, why are we having a button when its a link 🤔

@bodhish the button component does wrap it in a Link component if href is present

abhip161 · 2024-10-22T09:11:11Z

hey @rithviknishad could you check this out?

why are we having a button when its a link ??
mentioned that we're using a button, but it's meant for navigation (like a link). Should we wrap it inside a component instead of using directly?

abhip161 · 2024-10-22T21:35:12Z

hey @rithviknishad
Since ButtonV2 automatically handles the link behavior when href is provided,
don't need to manually wrap the button in a Link

<ButtonV2 id="manage-bed-button" variant="secondary" border className="mt-3 flex w-full items-center justify-between" href={location/${id}/beds} disabled={totalBeds == null} > Manage Beds <span className="flex items-center justify-center gap-2"> <CareIcon icon="l-bed" className="text-lg" /> {totalBeds ?? "--"} </span> </ButtonV2>

Let me know if this looks good!

Co-authored-by: Bodhish Thomas <[email protected]>

github-actions · 2024-10-23T10:13:19Z

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

Fixes greyed out 'Active Manage Bed' button in Location Management Page

b8ae7f1

abhip161 requested a review from a team as a code owner October 18, 2024 13:54

nihal467 added the needs testing label Oct 18, 2024

Merge branch 'develop' into issues/8814/fix-active-manage-bed-button

6460119

rithviknishad reviewed Oct 18, 2024

View reviewed changes

rithviknishad added the question Further information is requested label Oct 18, 2024

bodhish removed the question Further information is requested label Oct 21, 2024

abhip161 added 2 commits October 21, 2024 20:28

Refactored: Removed conditional classes, set button as disabled if to…

cb0d861

…talBeds is null

Merge branch 'issues/8814/fix-active-manage-bed-button' of https://gi…

fcb1fb9

…thub.com/abhip161/care_fe into issues/8814/fix-active-manage-bed-button

nihal467 added needs review tested and removed needs testing labels Oct 21, 2024

bodhish requested changes Oct 22, 2024

View reviewed changes

Update src/Components/Facility/LocationManagement.tsx

bf88c2f

Co-authored-by: Bodhish Thomas <[email protected]>

rithviknishad approved these changes Oct 23, 2024

View reviewed changes

rithviknishad requested a review from bodhish October 23, 2024 04:01

abhip161 added 2 commits October 23, 2024 10:59

Fix: Removed cn and wrapped Manage Beds button in Link with dynamic href

fd2f8ce

Fix: Removed cn and wrapped Manage Beds button in Link with dynamic href

e3515fb

github-actions bot added the merge conflict pull requests with merge conflict label Oct 23, 2024

bodhish approved these changes Oct 23, 2024

View reviewed changes

bodhish added reviewed reviewed by a core member and removed needs review labels Oct 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes greyed out 'Active Manage Bed' button in Location Management Page #8814 #8827

Fixes greyed out 'Active Manage Bed' button in Location Management Page #8814 #8827

abhip161 commented Oct 18, 2024

netlify bot commented Oct 18, 2024 •

edited

Loading

rithviknishad left a comment

rithviknishad Oct 18, 2024

abhip161 Oct 18, 2024 •

edited

Loading

bodhish Oct 21, 2024

abhip161 commented Oct 21, 2024

rithviknishad commented Oct 21, 2024 •

edited

Loading

abhip161 commented Oct 21, 2024

rithviknishad commented Oct 21, 2024

abhip161 commented Oct 21, 2024

rithviknishad commented Oct 21, 2024 •

edited

Loading

abhip161 commented Oct 21, 2024

samholics commented Oct 21, 2024

nihal467 commented Oct 21, 2024

bodhish Oct 22, 2024

rithviknishad Oct 22, 2024

abhip161 commented Oct 22, 2024

abhip161 commented Oct 22, 2024 •

edited

Loading

github-actions bot commented Oct 23, 2024

	totalBeds != null && "opacity-100",
	totalBeds == null && "opacity-50",

Fixes greyed out 'Active Manage Bed' button in Location Management Page #8814 #8827

Are you sure you want to change the base?

Fixes greyed out 'Active Manage Bed' button in Location Management Page #8814 #8827

Conversation

abhip161 commented Oct 18, 2024

Proposed Changes

Merge Checklist

netlify bot commented Oct 18, 2024 • edited Loading

✅ Deploy Preview for care-ohc ready!

rithviknishad left a comment

Choose a reason for hiding this comment

rithviknishad Oct 18, 2024

Choose a reason for hiding this comment

abhip161 Oct 18, 2024 • edited Loading

Choose a reason for hiding this comment

bodhish Oct 21, 2024

Choose a reason for hiding this comment

abhip161 commented Oct 21, 2024

rithviknishad commented Oct 21, 2024 • edited Loading

abhip161 commented Oct 21, 2024

rithviknishad commented Oct 21, 2024

abhip161 commented Oct 21, 2024

rithviknishad commented Oct 21, 2024 • edited Loading

abhip161 commented Oct 21, 2024

samholics commented Oct 21, 2024

nihal467 commented Oct 21, 2024

bodhish Oct 22, 2024

Choose a reason for hiding this comment

rithviknishad Oct 22, 2024

Choose a reason for hiding this comment

abhip161 commented Oct 22, 2024

abhip161 commented Oct 22, 2024 • edited Loading

github-actions bot commented Oct 23, 2024

netlify bot commented Oct 18, 2024 •

edited

Loading

abhip161 Oct 18, 2024 •

edited

Loading

rithviknishad commented Oct 21, 2024 •

edited

Loading

rithviknishad commented Oct 21, 2024 •

edited

Loading

abhip161 commented Oct 22, 2024 •

edited

Loading