-
-
Notifications
You must be signed in to change notification settings - Fork 32k
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
[TabScrollButton] Add ability to change left and right icons #33863
[TabScrollButton] Add ability to change left and right icons #33863
Conversation
Netlify deploy previewhttps://deploy-preview-33863--material-ui.netlify.app/ @material-ui/core: parsed: +0.14% , gzip: +0.14% Bundle size report |
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 basically have the same comment as on the other PR. We need to also have componentsProps
for allowing adding additional props. @pratikkarad in order to speed up the review process & not have many distraction, can we focus on getting one of these PRs to the finish line first, and then follow up with the others.
Sure, no problem 👍🏻 |
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 apologize for the delayed review. I've left some comments, please have a look.
|
||
const theme = useTheme(); | ||
const isRtl = theme.direction === 'rtl'; | ||
|
||
const ownerState = { isRtl, ...props }; | ||
|
||
const classes = useUtilityClasses(ownerState); | ||
const ScrollButtonStart = components.ScrollButtonStart; |
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.
const ScrollButtonStart = components.ScrollButtonStart; | |
const ScrollButtonStart = components.ScrollButtonStart ?? KeyboardArrowLeft; |
|
||
const theme = useTheme(); | ||
const isRtl = theme.direction === 'rtl'; | ||
|
||
const ownerState = { isRtl, ...props }; | ||
|
||
const classes = useUtilityClasses(ownerState); | ||
const ScrollButtonStart = components.ScrollButtonStart; | ||
const ScrollButtonEnd = components.ScrollButtonEnd; |
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.
const ScrollButtonEnd = components.ScrollButtonEnd; | |
const ScrollButtonEnd = components.ScrollButtonEnd ?? KeyboardArrowRight; |
components = { | ||
ScrollButtonStart: KeyboardArrowLeft, | ||
ScrollButtonEnd: KeyboardArrowRight, | ||
}, |
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.
components = { | |
ScrollButtonStart: KeyboardArrowLeft, | |
ScrollButtonEnd: KeyboardArrowRight, | |
}, | |
components = {}, |
The main value with this is that, if developer apply only one, it would crash the component
|
||
const theme = useTheme(); | ||
const isRtl = theme.direction === 'rtl'; | ||
|
||
const ownerState = { isRtl, ...props }; | ||
|
||
const classes = useUtilityClasses(ownerState); | ||
const ScrollButtonStart = components.ScrollButtonStart; | ||
const ScrollButtonEnd = components.ScrollButtonEnd; | ||
const getDefaultProps = (icon) => { |
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.
const getDefaultProps = (icon) => { | |
const getIconProps = (icon) => { |
@@ -67,9 +94,9 @@ const TabScrollButton = React.forwardRef(function TabScrollButton(inProps, ref) | |||
{...other} | |||
> | |||
{direction === 'left' ? ( | |||
<KeyboardArrowLeft fontSize="small" /> | |||
<ScrollButtonStart {...getDefaultProps(ScrollButtonStart)} /> |
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.
<ScrollButtonStart {...getDefaultProps(ScrollButtonStart)} /> | |
<ScrollButtonStart {...getIconProps(ScrollButtonStart)} /> |
) : ( | ||
<KeyboardArrowRight fontSize="small" /> | ||
<ScrollButtonEnd {...getDefaultProps(ScrollButtonEnd)} /> |
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.
<ScrollButtonEnd {...getDefaultProps(ScrollButtonEnd)} /> | |
<ScrollButtonEnd {...getIconProps(ScrollButtonEnd)} /> |
@@ -48,5 +50,33 @@ describe('<TabScrollButton />', () => { | |||
); | |||
expect(getAllByTestId('KeyboardArrowRightIcon').length).to.equal(1); | |||
}); | |||
|
|||
it('should render with the ArrowBackIcon icon', () => { |
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('should render with the ArrowBackIcon icon', () => { | |
it('should render with the custom start icon', () => { |
expect(getAllByTestId('ArrowBackIcon').length).to.equal(1); | ||
}); | ||
|
||
it('should render with the ArrowForwardIcon icon', () => { |
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('should render with the ArrowForwardIcon icon', () => { | |
it('should render with the end icon', () => { |
components = { | ||
ScrollButtonStart: KeyboardArrowLeft, | ||
ScrollButtonEnd: KeyboardArrowRight, | ||
}, |
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.
components = { | |
ScrollButtonStart: KeyboardArrowLeft, | |
ScrollButtonEnd: KeyboardArrowRight, | |
}, | |
components = {}, |
Hi @mnajdova, I made changes in PR as suggested. I also tested locally it is working fine but failing on |
Hi @mnajdova, As per my understanding, we have removed the initialization of components prop from Tab.js
to
that's why Argo is complaining about this because the start & end icon was hard coded before. Now it is trying to compare the before and after snapshots. |
I tried initializing the component prop in
As I suggested, I think initializing the component prop with default Icons may solve the issue. Any suggestions on this? |
Regression tests fail because they in your branch use the old SDK that's not supported anymore. Please marge in the latest master. |
Signed-off-by: Marija Najdova <mnajdova@gmail.com>
cb5724e
to
452d117
Compare
Hi @mnajdova, I've updated PR, PTAL. |
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.
@pratikkarad we've updated the props name to slots
and slotProps
accordingly. Would be great if we can follow this new convention. Also, I would use the description we have in the other components for these props, so that we are consistent. From technical point of view, I don't have any comments 👍
Hi @mnajdova, As I understand we only need a naming change: |
@safeamiiir Please go ahead. |
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 made the appropriate and required changes.
Supported callback in slotsProps, improved unit tests, added TypeScript tests, and updated the documentation.
It looks good to me now!
Closes #33235