-
Notifications
You must be signed in to change notification settings - Fork 0
Modify Tabs and create demo panel
#129
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
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #129 +/- ##
==========================================
+ Coverage 92.17% 92.21% +0.03%
==========================================
Files 86 86
Lines 1496 1503 +7
Branches 225 231 +6
==========================================
+ Hits 1379 1386 +7
Misses 98 98
Partials 19 19
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…panel_8 # Conflicts: # chartlets.js/CHANGES.md
b-yogesh
left a comment
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.
|
@b-yogesh: Did you run |
Yes, I forgot that, it works fine now. |
b-yogesh
left a comment
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.
It works. I added a couple of Box components following the MUI examples shown here: https://mui.com/material-ui/react-tabs/#BasicTabs.tsx.
Although it looks like the suggested change is quite a lot, it is just a couple of lines. If you agree, please incorporate these changes.
forman
left a comment
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.
I don't want to block merging, but the code readability can be improved (see my comments). Maybe later.
Otherwise good, so I approve!
| <MuiBox sx={{ borderBottom: 1, borderColor: "divider" }}> | ||
| <MuiTabs id={id} style={style} value={value} onChange={handleChange}> | ||
| {tabItems?.map((tab, index) => { | ||
| const tabState = isComponentState(tab) |
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.
You can make this cleaner code if you introduce a type guard isTabState().
| } | ||
| disabled={disabled || (tabState && tabState.disabled)} | ||
| /> | ||
| ); |
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.
You can make this cleaner by constructing separate MuiTab components, one for the case where !tabState and the other where you have it.
Even better would be to ensure / assert upfront whether tabItems contains only TabState instances.

This PR:
Tabscomponent.Tabscomponent