feat: Add reusable BottomSheet component - #1653
Conversation
Signed-off-by: KhushamBansal <kbkhushambansal@gmail.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new mobile-friendly BottomSheet component that slides up from the bottom of the screen. The feedback recommends exposing the close button's aria-label as a configurable prop (closeButtonAriaLabel) instead of hardcoding the string 'Close' to support internationalization (i18n) and localization.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Signed-off-by: KhushamBansal <kbkhushambansal@gmail.com>
Signed-off-by: KhushamBansal <kbkhushambansal@gmail.com>
There was a problem hiding this comment.
Goooooood work here. 🥇 Let's reinforce this work and make it super solid, defined once, reused everywhere by ensuring:
- no deviances in its use should be necessary; no maintenance need in BottomSheet or in the components that use it. This should be a one-time effort. For example, given its use in the display of mui-datatable column checkboxes or mui-datatable filter choices, the instances of those mui-datatables should not need to wire up to the BottomSheet - this shall be dynamic; neither component should require no consideration on an ongoing basis in either BottomSheet or in mui-datatables.
- How does this compare to what we have in meshery-cloud on the /catalog page?
- Is there an open issue to migrate that implementation to a centralized component?
- What learnings are being taken from the component in meshery-cloud?
- In what ways is this the best possible version of a "BottomSheet" component? Were are our examples of mui-datatables using this component to display filters?
- Is there an open issue to centrally and pervasively incorporate this BottomSheet component into use of each and every mui-dtatatable and to have code written once-only that defines when to activate and deactivate the BottomSheet (so the mobile experience is simply implicit)?
Answer here and/or link to each of the GH issues that you open in response to these items. Don't merge this unless my requirements above are being met.
|
@leecalcote @aabidsofi19
Conclusion
|
|
Consider using the same approach used for other components. |
|
Thanks @leecalcote for your suggestion. I found that we can also use the If you think we should go ahead with Screen.Recording.2026-06-25.at.12.23.30.PM.mov |
|
@KhushamBansal Thank you for your contribution! Let's discuss this during the website call tomorrow at 5:30 PM IST | 7 AM CST Add it as an agenda item to the meeting minutes, if you would 🙂 |
@leecalcote Which approach would you recommend? |
Signed-off-by: Khusham Bansal <142911972+KhushamBansal@users.noreply.github.com>
|
@KhushamBansal I'd prefer sticking with the Modal component as it is already being used in Meshery UI . If you cannot find other usecases where we can use the BottomSheet component, Modal component should be it. |
|
@PARTH-TUSSLE We don't currently have a BottomSheet(or similar) component in Sistent. It was discussed in the meeting that we should add one, as it can be reused across multiple components. |
Then it might make sense to add another component and I took a look at meshery/meshery#20188 this PR which ig already solves this issue. So would you then replace the current fix to use the BottomSheet component? |
Signed-off-by: KhushamBansal <kbkhushambansal@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a controlled MUI-based ChangesBottom sheet component
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Consumer
participant BottomSheet
participant Dialog
participant SlideUp
participant IconButton
Consumer->>BottomSheet: Set open and provide content
BottomSheet->>Dialog: Render dialog with open and onClose
Dialog->>SlideUp: Animate from bottom
Dialog-->>Consumer: Display header and scrollable content
Consumer->>IconButton: Activate close control
IconButton->>BottomSheet: Invoke onClose
BottomSheet-->>Consumer: Report close event
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/custom/index.tsx (1)
186-194: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDuplicate
export * from './LiquidGlass';line.Line 186 already re-exports
./LiquidGlass; line 193 duplicates it verbatim before the new./BottomSheetexport on line 194. This looks like a leftover from a bad merge/copy when adding the BottomSheet export.🧹 Proposed fix
export * from './Workspaces'; -export * from './LiquidGlass'; export * from './BottomSheet';🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/custom/index.tsx` around lines 186 - 194, Remove the duplicate `export * from './LiquidGlass';` entry from the module export list, keeping the existing `LiquidGlass` re-export and the new `BottomSheet` export intact.src/custom/Modal/index.tsx (1)
161-172: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winStale comment contradicts the new
omit(props, ['fullWidth'])call.The comment states that stripping
fullWidthis unnecessary because the explicitfullWidth={!fullScreen}prop applied later (line 189) always wins ("that explicit prop is applied after {...restProps}... intentionally overridden without needing to strip it here"), yet the code right below now explicitly strips it viaomit. Either the comment is outdated and should explain why omitting was still added (e.g., to keeprestProps's inferred type from carrying afullWidthkey), or theomitcall is redundant given the later override and can be dropped. Leaving both as-is will confuse future readers about the actual rationale.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/custom/Modal/index.tsx` around lines 161 - 172, Resolve the contradiction between the comment and the destructuring in the Modal component: either remove the redundant fullWidth omission from omit(props, ['fullWidth']) or update the comment to accurately document why fullWidth is intentionally omitted, such as preserving the inferred restProps type. Keep the StyledDialog fullWidth override behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/custom/BottomSheet/BottomSheet.tsx`:
- Around line 64-101: Update the BottomSheet header styling to use semantic
Sistent theme palette tokens for both the default header text color and the
close icon fill. Replace the raw `#eee` fallback in the header Box and hardcoded
`#fff` SVG fill in the IconButton styles, while preserving the existing explicit
headerTextColor override and theme-based background behavior.
- Line 8: Remove the unused Divider import from BottomSheet.tsx while leaving
the rest of the BottomSheet implementation unchanged.
---
Outside diff comments:
In `@src/custom/index.tsx`:
- Around line 186-194: Remove the duplicate `export * from './LiquidGlass';`
entry from the module export list, keeping the existing `LiquidGlass` re-export
and the new `BottomSheet` export intact.
In `@src/custom/Modal/index.tsx`:
- Around line 161-172: Resolve the contradiction between the comment and the
destructuring in the Modal component: either remove the redundant fullWidth
omission from omit(props, ['fullWidth']) or update the comment to accurately
document why fullWidth is intentionally omitted, such as preserving the inferred
restProps type. Keep the StyledDialog fullWidth override behavior unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: cee10d6c-62c7-4975-85a4-97abb10d101f
📒 Files selected for processing (4)
src/custom/BottomSheet/BottomSheet.tsxsrc/custom/BottomSheet/index.tssrc/custom/Modal/index.tsxsrc/custom/index.tsx
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Khusham Bansal <142911972+KhushamBansal@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Khusham Bansal <142911972+KhushamBansal@users.noreply.github.com>
|
Can you merge master again? This version of sistent is not compatible with latest meshery. |
There was a problem hiding this comment.
Please run prettier on the newly added files.
Apart from that, animations don't work on Firefox for some reason, but I think that the issue might be somewhere else. If the fix isn't within the current files, it might be a good idea to raise an issue for this once this is merged.
2026-07-28.16-10-44.mp4
|
For other reviewers wanting to preview this component, feel free to clone this branch. It implements BottomSheet. |
Signed-off-by: KhushamBansal <kbkhushambansal@gmail.com>
|
@Bharath314 I have made all the suggested changes. Thanks for reviewing. |
7 participants
Notes for Reviewers
This PR fixes #
Dark mode:
Screen.Recording.2026-07-16.at.8.30.14.PM.mov
Light mode:
Screen.Recording.2026-07-16.at.8.31.07.PM.mov
Signed commits
Summary by CodeRabbit