poc(Button): display svg icon path on button hover#364
poc(Button): display svg icon path on button hover#364miroslavstastny wants to merge 2 commits intomasterfrom
Conversation
miroslavstastny
requested review from
dvdzkwsk,
kuzhelov,
levithomason and
mnajdova
as code owners
| ), | ||
| styles: { | ||
| svg: () => ({ | ||
| color: 'inherit', |
There was a problem hiding this comment.
This works better with button hover, see circular button example with this icon vs call icon
| export default { | ||
| icon: ({ classes }) => ( | ||
| <svg viewBox="0 0 32 32" role="presentation" className={classes.svg}> | ||
| <g className="icons-default-fill"> |
There was a problem hiding this comment.
Not sure if we need this class for anything.
There was a problem hiding this comment.
We don't need this for icons in Stardust. I am going to be importing all the icons without this class
| horizontalSpace: string | ||
| margin: string | ||
| secondaryColor: string | ||
| filled?: boolean |
There was a problem hiding this comment.
probably, we should think about making this part of the Icon's interface
| /> | ||
| <ComponentExample | ||
| title="Toolbar button" | ||
| description="Icon paths change on hover" |
There was a problem hiding this comment.
nit: just for the sake of consistency, there should be dot at the end ;)
kuzhelov
left a comment
kuzhelov
left a comment
There was a problem hiding this comment.
do like the approach - the only problem, probably, an apparent one, is that there are quite a lot of indirections involved here. At the same time, worth to notice that this PR reduces, arguably, their amount to the best possible one. Also, it seems that there are some ways to further encapsulate idea of this approach under more convenient generic API exposed by utility methods
|
@codepretty - could you please review this to verify it is in line with your changes/plans on SVG icons? |
Codecov Report
@@ Coverage Diff @@
## master #364 +/- ##
=======================================
Coverage 91.77% 91.77%
=======================================
Files 41 41
Lines 1325 1325
Branches 169 169
=======================================
Hits 1216 1216
Misses 105 105
Partials 4 4Continue to review full report at Codecov.
|
| iconOnly | ||
| text | ||
| styles={{ | ||
| '&:hover .ui-icon__filled': { |
There was a problem hiding this comment.
Wouldn't you also need to hide the unfilled version?
|
|
||
| secondaryPath: getSvgStyle('secondaryPath'), | ||
|
|
||
| unfilled: getSvgStyle('unfilled'), |
There was a problem hiding this comment.
I am implementing this in my PR where we won't need to have these styles defined in each SVG. I also named them filled & outline. Do you think filled & unfilled works better?
There was a problem hiding this comment.
I think filled & outline are fine, unfilled was used in legacy client.
|
Closing this for now, will create a new PR once #478 is merged. |
4 participants
This is just an experiment to learn how
ButtonwithIconis supposed to be used in order to implement button which contains an icon which is 'filled' on hover.Requirements (@200%):
Proposed implementation concerns
ui-icon__filled)