-
Notifications
You must be signed in to change notification settings - Fork 588
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
fix modal navigation bug #4803
fix modal navigation bug #4803
Conversation
WalkthroughThe changes consist of updates to the Changes
Possibly related PRs
Poem
Warning Review ran into problemsProblemsGit: Failed to clone repository. Please run the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
app/packages/core/src/components/Modal/ModalSamplePlugin.tsx (1)
Line range hint
23-88
: Consider extracting the tooltip functionality into a separate component.The
ModalSample
component currently handles the tooltip functionality, which is not directly related to the modal's core purpose. Consider extracting the tooltip-related logic, including thetooltipEventHandler
,useEffect
hooks, and related Recoil state management, into a separate component. This will help further decouple the modal from the tooltip functionality and improve the component's adherence to the single responsibility principle.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (3)
- app/packages/core/src/components/Modal/Modal.tsx (2 hunks)
- app/packages/core/src/components/Modal/ModalNavigation.tsx (1 hunks)
- app/packages/core/src/components/Modal/ModalSamplePlugin.tsx (1 hunks)
Additional context used
Path-based instructions (3)
app/packages/core/src/components/Modal/ModalSamplePlugin.tsx (1)
Pattern
**/*.{ts,tsx}
: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.app/packages/core/src/components/Modal/ModalNavigation.tsx (1)
Pattern
**/*.{ts,tsx}
: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.app/packages/core/src/components/Modal/Modal.tsx (1)
Pattern
**/*.{ts,tsx}
: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
Additional comments not posted (4)
app/packages/core/src/components/Modal/ModalSamplePlugin.tsx (1)
Line range hint
1-6
: Approve the removal of unused imports and components.The removal of unused imports and components related to
ModalNavigation
helps simplify the modal's structure and focus on its core purpose of rendering the appropriate child component based on the state.Also applies to: 8-11
app/packages/core/src/components/Modal/ModalNavigation.tsx (1)
29-29
: Approve the change in the arrow's positioning, but consider responsiveness.The change in the
bottom
property of theArrow
component from50%
to33vh
will position the arrow consistently at one-third of the viewport height from the bottom, making it more easily accessible to users.However, please consider testing the responsiveness and visual consistency of this change across various screen sizes and devices to ensure that the arrow's position remains appropriate relative to the modal's content.
app/packages/core/src/components/Modal/Modal.tsx (2)
12-12
: LGTM!The import statement for
ModalNavigation
is valid and follows the correct syntax.
167-168
: LGTM!The usage of the
useLookerHelpers
hook to access theonNavigate
function follows the React hooks pattern and aligns with best practices.
.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
ModalSample
component by removing unused imports and simplifying layout.