fix: make Launching Soon modal reachable by adding trigger action#1104
Conversation
|
Thank you @, for creating the PR and contributing to our UltimateHealth project 💗. |
|
/review |
🤖 Gemini AI Code ReviewSummaryThis Pull Request aims to fix an issue where the "Launching Soon" modal was never displayed due to a missing trigger. The author has implemented a mechanism to show this modal when a specific UI element (the "Admin Panel" button for Android) is interacted with. The PR successfully introduces the trigger for the modal and improves the semantic HTML for the button. However, it also introduces a significant functional change by replacing a direct link to the Google Play Store with the modal, which requires explicit confirmation of intent. 🔴 High Severity
🟡 Medium SeverityNo medium severity issues identified. 🟢 Low Severity / Nits
What's Good ✅
VerdictRequest Changes: The high-severity issue regarding the functional change of the "UHealth Admin" button requires immediate attention and clarification. This is a significant alteration to user flow that needs explicit confirmation before merging. |
SB2318
left a comment
There was a problem hiding this comment.
Thanks for adding the missing trigger for the "Launching Soon" modal.
However, I noticed that the existing "UHealth Admin" action appears to have been changed from a direct Google Play Store link to a button that only opens the modal.
Could you clarify whether this behavior change was intentional and approved?
From the previous implementation, users could directly navigate to the UHealth Admin Play Store page. With this PR, that navigation path seems to be removed entirely and replaced by the modal. Since the issue is about making the modal reachable, this introduces a broader functional change than the scope described in the PR.
If the app is genuinely not available yet, replacing the link may be appropriate. Otherwise, we should consider preserving the existing navigation behavior or documenting this change explicitly in the PR description.
Also, please confirm whether the removed Play Store link was intentionally deprecated.
….com/Krishnx21/UltimateHealth into fix-unreachable-launching-soon-modal
|
@SB2318 thank you for reviewing i have updated it . |
|
/review |
🤖 Gemini AI Code ReviewSummaryThis PR aims to resolve an issue where the "Launching Soon" modal was inaccessible by introducing a trigger to update its state. The core change involves adding an While the primary intent of the PR is clear and the solution for the modal trigger appears functionally correct, the PR contains several significant unrelated changes and artifacts that severely impact its quality and reviewability. 🔴 High Severity
🟡 Medium Severity
🟢 Low Severity / Nits
What's Good ✅
VerdictRequest Changes: The presence of a high-severity, unrelated Git submodule change and unresolved merge conflicts makes this PR unmergeable and requires significant rework. The unrelated layout component removals also need to be addressed. |
Post-Approval Review Findings🔴 Blocking: Unresolved Merge Conflicts
This leaves the file in an invalid state and will prevent successful builds and merges. Please resolve the conflicts and ensure only the intended final code remains. 🔴 Blocking: Unrelated Submodule ModificationThis PR includes an update/addition to the The stated purpose of this PR is to make the "Launching Soon" modal reachable. The submodule change appears unrelated to that goal and significantly increases the review scope. Please remove the submodule modification from this PR and submit it separately if it is required. 🟡 Needs Clarification: Login Layout ChangesThe PR removes These changes appear unrelated to the modal accessibility fix. If they are intentional, please explain the reason and confirm there are no styling, layout, or accessibility regressions. Otherwise, consider reverting them to keep the PR focused on a single concern. 🟢 Minor: Import CleanupThere appears to be a duplicate This may be a byproduct of the unresolved merge conflict. After resolving conflicts, please verify imports are cleaned up and deduplicated. ✅ What's Good
SummaryThe primary feature implementation looks reasonable and appears to solve the original issue. However, the presence of unresolved merge conflicts and unrelated submodule modifications makes the PR unmergeable in its current state. Please address the blocking issues and provide clarification for the login page layout changes before resubmission. |
|
@SB2318 THANKS FOR REVIEWING
|
|
/review |
🤖 Gemini AI Code ReviewSummaryThis Pull Request aims to resolve an issue where the "Launching Soon" modal was inaccessible to users by introducing a trigger to update its state. The core logic for enabling the modal via a new button and prop passing is sound and well-implemented. However, the PR contains critical merge conflict markers in 🔴 High Severity
🟡 Medium Severity
🟢 Low Severity / Nits
What's Good ✅
VerdictRequest Changes The presence of critical merge conflict markers and duplicate components in |
|
Congratulations, Your pull request has been successfully merged 🥳🎉 Thank you for your contribution to the project 🚀 Keep Contributing!! ✨ |
Description
This PR fixes the issue where the "Launching Soon" modal could never be displayed because
comingSoonModalwas initialized asfalseand there was no code path that updated it totrue.Changes Made
setComingSoonModal(true)when users interact with the intended UI element.Problem
Previously:
The modal was conditionally rendered based on
comingSoonModal, but the state was never updated totrue, making the modal unreachable for users.Solution
Implemented the missing trigger so the modal state is updated appropriately and the feature is now accessible.
Related Issue
Closes #1046
Testing