-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Tabs icon toggle #25597
Tabs icon toggle #25597
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
{ | ||
"type": "patch", | ||
"comment": "Added support for regular/filled icon toggling", | ||
"packageName": "@fluentui/react-tabs", | ||
"email": "[email protected]", | ||
"dependentChangeType": "patch" | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,6 +16,13 @@ const reservedSpaceClassNames = { | |
content: 'fui-Tab__content--reserved-space', | ||
}; | ||
|
||
// These should match the constants defined in @fluentui/react-icons | ||
// This package avoids taking a dependency on the icons package for only the constants. | ||
const iconClassNames = { | ||
filled: 'fui-Icon-filled', | ||
regular: 'fui-Icon-regular', | ||
}; | ||
|
||
/** | ||
* Styles for the root slot | ||
*/ | ||
|
@@ -360,6 +367,12 @@ const useIconStyles = makeStyles({ | |
display: 'inline-flex', | ||
justifyContent: 'center', | ||
...shorthands.overflow('hidden'), | ||
[`& .${iconClassNames.filled}`]: { | ||
display: 'none', | ||
}, | ||
[`& .${iconClassNames.regular}`]: { | ||
display: 'inline', | ||
}, | ||
}, | ||
// per design, the small and medium font sizes are the same. | ||
// the size prop only affects spacing. | ||
|
@@ -378,6 +391,14 @@ const useIconStyles = makeStyles({ | |
height: '24px', | ||
width: '24px', | ||
}, | ||
selected: { | ||
[`& .${iconClassNames.filled}`]: { | ||
display: 'inline', | ||
}, | ||
[`& .${iconClassNames.regular}`]: { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This makes me a bit worried, but your reasoning does make sense and I suppose it'd be easy enough to take a dependency on react-icons in the future if it becomes a problem. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes - always easier to add the dependency than take it away :) |
||
display: 'none', | ||
}, | ||
}, | ||
}); | ||
|
||
/** | ||
|
@@ -463,7 +484,13 @@ export const useTabStyles_unstable = (state: TabState): TabState => { | |
); | ||
|
||
if (state.icon) { | ||
state.icon.className = mergeClasses(tabClassNames.icon, iconStyles.base, iconStyles[size], state.icon.className); | ||
state.icon.className = mergeClasses( | ||
tabClassNames.icon, | ||
iconStyles.base, | ||
iconStyles[size], | ||
selected && iconStyles.selected, | ||
state.icon.className, | ||
); | ||
} | ||
|
||
// This needs to be before state.content.className is updated | ||
|
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 think react-tabs should avoid a dependency on react-icons at least for now so I'm willing to manage declaring the constant here. Filed this issue to track fixing it: #25598