-
-
Notifications
You must be signed in to change notification settings - Fork 33
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
Add "borders" option #537
base: main
Are you sure you want to change the base?
Add "borders" option #537
Conversation
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.
Thanks for the contribution. I've left some comments to address.
@@ -73,6 +73,7 @@ decimal by 1). Otherwise, the integration may complain of a duplicate unique ID. | |||
| `offset` | number | **Optional** | Number of forecast segments to offset from start | `0` | | |||
| `label_spacing` | number | **Optional** | Space between time/temperature labels (integer >= 1) | `2` | | |||
| `colors` | [object][color] | **Optional** | Set colors for all or some conditions | | | |||
| `borders` | [color] | **Optional** | Set color for a border around segments. Defaults to null (no border) | | |
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.
| `borders` | [color] | **Optional** | Set color for a border around segments. Defaults to null (no border) | | | |
| `borders` | [color] | **Optional** | Set color for a border around segments. | `null` (no border) | |
.shadow() | ||
.find('div.bar > div.bar-span') | ||
.each((el) => { | ||
cy.wrap(el).should('have.css', 'border-color', 'rgb(50, 205, 50)'); | ||
}); |
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.
.shadow() | |
.find('div.bar > div.bar-span') | |
.each((el) => { | |
cy.wrap(el).should('have.css', 'border-color', 'rgb(50, 205, 50)'); | |
}); | |
.shadow() | |
.find('div.bar > div.bar-span') | |
.each((el) => { | |
cy.wrap(el).should('have.css', 'border-color', 'rgb(50, 205, 50)'); | |
}); |
if (this.borders) { | ||
borderStyles = this.getBarSpanStyles(this.borders); | ||
} | ||
console.log(borderStyles); |
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.
Remove debug logging
console.log(borderStyles); |
border-style: solid; | ||
border-width: 1px; | ||
border-color: transparent |
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.
Does this impact the rendering when there are no borders requested? We may have to only set the border width when borders are configured to avoid adding a 1px space around color blocks by default.
@@ -393,6 +393,7 @@ export class HourlyWeatherCard extends LitElement { | |||
.precipitation=${precipitation} | |||
.icons=${!!config.icons} | |||
.colors=${colorSettings.validColors} | |||
.borders=${config.borders} |
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.
Like the colors
config, we must validate the input here before using it. Otherwise, this opens a security hole for folks to inject arbitrary persisted script in the card CSS. You should be able to reuse the color validation logic that we already have.
@@ -322,6 +347,7 @@ export class WeatherBar extends LitElement { | |||
grid-area: right; | |||
border: 1px solid var(--divider-color, lightgray); | |||
border-width: 0 0 0 1px; | |||
border-collapse: collapse; |
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.
This may lead to the axis marks being too narrow. Can we double check that the axis ticks remain the same after this change?
The `borders` config option is added which allows setting a rendered border for bar `span` elements. This makes them more visible when used without colors on things like e-ink displays.
The
borders
config option is added which allows setting a rendered border for barspan
elements. This makes them more visible when used without colors on things like e-ink displays.An example of the effect (when used with colors removed):
The current implementation simple allows specifying a color for the borders, and hard codes a 1px bounding box. This is under the
borders
key.Possibly the borders key should be more flexible and permit several input modes - i.e.
borders: true
--> black bordersborders: "green"
--> borders rendered as single colorYields borders with more CSS set
and then finally something more like the "colors" option which would allow per-type border setting (not sure how useful this really would be though).
Alternately this should be refactored under the
colors
option, although in that case I would like to do it so something like:was a valid input option to quickly set globals.
Closes #536