Skip to content
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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add "borders" option #537

wants to merge 1 commit into from

Conversation

wrouesnel
Copy link

@wrouesnel wrouesnel commented Oct 8, 2023

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.

An example of the effect (when used with colors removed):

image

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 borders

borders: "green" --> borders rendered as single color

borders:
  width: 4px
  color: limegreen

Yields 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:

colors:
  borders: black
  background-color: white
  foreground-color: transparent

was a valid input option to quickly set globals.

Closes #536

Copy link
Owner

@decompil3d decompil3d left a 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) | |
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
| `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) |

Comment on lines +140 to +144
.shadow()
.find('div.bar > div.bar-span')
.each((el) => {
cy.wrap(el).should('have.css', 'border-color', 'rgb(50, 205, 50)');
});
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
.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);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove debug logging

Suggested change
console.log(borderStyles);

Comment on lines +323 to +334
border-style: solid;
border-width: 1px;
border-color: transparent
Copy link
Owner

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}
Copy link
Owner

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;
Copy link
Owner

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bounding Boxes for Bar Sections
2 participants