Add refresh buttons to view title bars #22
Conversation
There was a problem hiding this comment.
🎉 this is great, thanks! some comments on component design inline. For the common-elements, not sure how inspectorCommon.css that's already included plays into that, maybe can start moving the relevant pieces into the DevtoolsIconButtonElement? What do you think?
| @@ -0,0 +1,83 @@ | |||
| export default ` | |||
There was a problem hiding this comment.
Most of these styles are already included via src/app/styles/inspectorCommon.css -- I like the idea of moving it with the component though. Is there any reason this is a separate file/string rather than in the style element of the component?
There was a problem hiding this comment.
While the icon styles are already in inspectorCommon.css, they weren't being applied to the DevtoolsIconButtonElement (is this a shadow-dom thing?). I put the styles into ui-icon.js so they can be used by anything that needs the sprite sheets.
| @@ -83,7 +83,9 @@ export default class MaterialViewElement extends BaseElement { | |||
| display: flex; | |||
| } | |||
| </style> | |||
| <title-bar title="Material View"></title-bar> | |||
| <title-bar title="Material View"> | |||
| <devtools-icon-button sheet-variant="large" icon-style="--spritesheet-position:-84px 48px; width: 28px; height: 24px;"> | |||
There was a problem hiding this comment.
Maybe these values of sheet-variant and icon-style can be the default styles?
Passing in a style string to a child component feels a bit like circumventing cascading styles, or a bit React-y -- setting the style here with CSS vars will cascade down to the devtools-icon-button's span, or if using these values as a default, then in the :host style for the icon, e.g.
<devtools-icon-button style="--spritesheet-position: -84px 48px;"></devtools-icon-button>
Or when overriding default size
<devtools-icon-button style="--spritesheet-position: -84px 48px; --button-width: 50px; --button-height: 50px"></devtools-icon-button>
Exposing CSS variables as an API and just using the cascade seems to fit the style of LitElement/web components; what do you think?
There was a problem hiding this comment.
I think I'll move the icon span into its own class and move the ui-icon.js styles to there as well. Maybe it could look something like <devtools-icon glyph="refresh"></devtools-icon> ?
|
I moved ui-icon into its own element The icons can now be accessed by name e.g. |
|
@jacobcoughenour sorry for the wait, looks great and thank you! |
Related to #7 #18 and #19
I ported the icons from chrome devtools to add a element. You have to use "--spritesheet-position" to get the desired icon for now. I had to tweak the icon styles from chrome to get them to work in firefox.
As a use case, I added "refresh" icon buttons to the title bars for some of the views. They currently don't do anything when clicked.