Dashboard style update #8962
Dashboard style update #8962
Conversation
|
|
||
| :host::before { | ||
| content: ''; | ||
| display: block; |
There was a problem hiding this comment.
Minor nit, but this is not required, as absolute/fixed positioned elements become block containers automatically.
| display: block; |
| * `--vaadin-dashboard-item-rowspan` | rowspan of the widget | ||
| * Custom Property | Description | ||
| * --------------------------------------------|---------------------- | ||
| * `--vaadin-dashboard-item-colspan` | colspan of the widget |
There was a problem hiding this comment.
Should we align the naming for all the properties to maybe use --vaadin-dashboard-item-xyz?
There was a problem hiding this comment.
Indeed, done in 93ddf24
There was a problem hiding this comment.
Or, rather, renamed -item- properties to -widget-, since they're all widgets anyway
|
We should probably add some visual tests for the themes. |
| * `--vaadin-dashboard-widget-shadow` | widget shadow (non edit mode) | ||
| * `--vaadin-dashboard-widget-editable-shadow` | widget edit mode shadow | ||
| * `--vaadin-dashboard-widget-selected-shadow` | selected widget shadow | ||
| * `--vaadin-dashboard-widget-padding` | padding around widget content |
There was a problem hiding this comment.
We should also add the new properties to the TS file.
| } | ||
|
|
||
| [part='title'] { | ||
| flex: 1; | ||
| color: var(--lumo-header-text-color); | ||
| margin: 0; | ||
| white-space: nowrap; | ||
| white-space: var(--vaadin-dashboard-widget-title-wrap, wrap); |
There was a problem hiding this comment.
This seems to be missing from the property list.
Added some tests. Can you check the baseline images @rolfsmeds ? |
As I've now modified stuff again, I'm guessing the baselines need updating too @ugur-vaadin ? |
Updated the images. Header content alignment seems to have changed. Can you check to see if anything looks out of place? |
Header content alignment was changed to support wrapping widget titles, but I see now that apparently the titles are rendering significantly lower in the test images than on macOS, which probably means they do so on Windows too. Doesn't look very good. I'll make a couple of small tweaks to hopefully make it look better. |
…etter visual alignment.
Updated the images. |
|
LGTM |
| @@ -85,8 +85,8 @@ export const DashboardLayoutMixin = (superClass) => | |||
| --_vaadin-dashboard-row-height: minmax(var(--_vaadin-dashboard-row-min-height, auto), auto); | |||
|
|
|||
| display: grid; | |||
| overflow: auto; | |||
| height: 100%; | |||
| overflow: hidden; | |||
There was a problem hiding this comment.
Why overflow: hidden here? Wouldn't the default visible work fine? I know it’s unlikely, but perhaps some developer really needs very large shadows on the cards, and makes sure they are visible by adding padding on the host. If this internal element hides overflow, the shadows will be clipped.
And now I realize that `--vaadin-dashboard-padding is on this element as well, not the host – why?
There was a problem hiding this comment.
Btw, I don't see the need to prefix the internal custom properties with vaadin-dashboard (e.g., --_vaadin-dashboard-default-row-min-width vs --_default-row-min-width). Just adds noise IMO.
There was a problem hiding this comment.
Looks like flat widgets still have a shadow, which it shouldn't.
There was a problem hiding this comment.
Similarly here, I wouldn't prefix the internal custom properties with vaadin-dashboard-widget.
There was a problem hiding this comment.
Remind me again how the elevated style should differ from the default style. Now I don't really see the difference.
| @@ -23,7 +23,7 @@ export const DashboardLayoutMixin = (superClass) => | |||
| return css` | |||
| :host { | |||
| display: block; | |||
| overflow: hidden; | |||
| overflow: auto; | |||
| width: 100%; | |||
There was a problem hiding this comment.
| width: 100%; | |
| box-sizing: border-box; | |
| width: 100%; |
Description
Fixes #8961
Fixes #8706
Type of change
Checklist
Additional for
Featuretype of change