-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Rnmobile/native toolbar component ui #11827
Changes from all commits
ff3cd91
e28480f
3cff075
6a06306
442a6eb
5c6ae4f
b60acce
565bd08
125a576
7588291
826bec4
c79a483
a5aa2f2
7c6c0f4
4aaac80
54440b6
ee7a909
2560918
2fa504d
66e210f
45643fa
4fed682
dc8c21a
0809e9f
fc6cf65
bdb66bf
1c7dfc4
0162da0
51956f8
746c90a
49e82b4
b22ad2e
56640cd
591ce94
0c81045
5e6e9d2
bfdab84
a9aa8e5
168f08b
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,4 @@ | ||
export const IconClass = ( props ) => { | ||
const { icon, className } = props; | ||
return [ 'dashicon', 'dashicons-' + icon, className ].filter( Boolean ).join( ' ' ); | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
export const IconClass = ( props ) => { | ||
const { icon, className, ariaPressed } = props; | ||
return [ ariaPressed ? 'dashicon-active' : 'dashicon', 'dashicons-' + icon, className ].filter( Boolean ).join( ' ' ); | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,6 +21,7 @@ import Dashicon from '../dashicon'; | |
class IconButton extends Component { | ||
render() { | ||
const { icon, children, label, className, tooltip, shortcut, labelPosition, ...additionalProps } = this.props; | ||
const { 'aria-pressed': ariaPressed } = this.props; | ||
const classes = classnames( 'components-icon-button', className ); | ||
const tooltipText = tooltip || label; | ||
|
||
|
@@ -42,7 +43,7 @@ class IconButton extends Component { | |
|
||
let element = ( | ||
<Button aria-label={ label } { ...additionalProps } className={ classes }> | ||
{ isString( icon ) ? <Dashicon icon={ icon } /> : icon } | ||
{ isString( icon ) ? <Dashicon icon={ icon } ariaPressed={ ariaPressed } /> : icon } | ||
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. Not sure I understand this change? why do we need aria-pressed here since we're applying it to the parent button? 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. The underlying issue is that we need to find a way to set the color of a Ideally we'd be able to set We just found something that worked to keep us moving although we were aware it wasn't great, so I'm happy to discuss a better solution 😁 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. I'm concerned about these small tweaks having an impact on the web code base, can't we create an 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.
@koke Can you point me to where this occurs? As best I can follow, the main impact of the I'd tend to agree with @youknowriad here. "Pressed" does not resound to me as a characteristic of an icon, and thus it does not make sense as a prop. It does, however, seem natural to describe the state of an 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. I think this is what's setting the style https://github.com/WordPress/gutenberg/blob/master/packages/components/src/primitives/svg/style.native.scss#L6 I agree that this isn't ideal. It seems like the introduction of 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. I don't know that #13977 is an improvement here, as I expect you'd continue to treat it as a pressed icon, which is the root of the issue ("pressed" is not a sensible state of an icon). 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. Ah, that's right. We keep bumping into this on native: the web can rely on nested CSS definition to pass style down to lower level components, but we can't do that unless we pass them explicitly. I still think @etoledom can you handle this (removing ariaPressed from Dashicon) as part of #13977? |
||
{ children } | ||
</Button> | ||
); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
.dashicon { | ||
color: #7b9ab1; | ||
fill: currentColor; | ||
} | ||
|
||
.dashicon-active { | ||
color: #fff; | ||
fill: currentColor; | ||
} | ||
|
||
.dashicons-insert { | ||
color: #87a6bc; | ||
fill: currentColor; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
.container { | ||
flex-direction: row; | ||
border-right-width: 1px; | ||
border-right-color: #e9eff3; | ||
padding-left: 5px; | ||
padding-right: 5px; | ||
} |
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.
Is this a component or or a function, if it's a function it should be lower-case.
Also, we shouldn't touch this file directly, it's synced from the Dashicon repository I think, which means these changes can get removed inadvertently.