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

feat: add show_legend_state #1082

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ljmerza
Copy link

@ljmerza ljmerza commented Feb 20, 2024

@ljmerza ljmerza changed the title add show_legend_state feat: add show_legend_state Feb 21, 2024
Copy link
Collaborator

@akloeckner akloeckner 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 suggestion! Looks like a useful feature.

I added a few remarks. I'll still have to test the feature before accepting it.

Please also direct your PR to the dev branch.

@@ -130,6 +130,7 @@ properties of the Entity object detailed in the following table (as per `sensor.
| unit | string | | Set a custom unit of measurement, overrides `unit` set in base config.
| aggregate_func | string | | Override for aggregate function used to calculate point on the graph, `avg`, `median`, `min`, `max`, `first`, `last`, `sum`.
| show_state | boolean | | Display the current state.
| show_legend_state | boolean | | Display the current state with the legend.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Even though this is not the case for most of the other entries: please state the default value as false. Let's be a good example. ;-)

</div>
`)}
${this.visibleLegends.map((entity) => {
let legend = this.computeName(entity.index);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please properly indent the code.


const { show_legend_state } = this.config.entities[entity.index];
if (show_legend_state) {
legend += ` (${this.computeState(state)}${this.computeUom(entity.index)})`;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you put the state in an additional span with a suitable class? (If there already are suitable classes for state and UOM, you could maybe reuse them.)

I think there is a CSS property then to add the braces to the content.

That way users could use card-mod to style the appearance of the state as they wish.

Please make sure the defaults look good in different situations, e.g. long and short state names.

Copy link
Collaborator

Choose a reason for hiding this comment

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

put the state in an additional span

Absolutely correct.

Also, please think about an order: in some cases a user may want to have UoM before a value.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think, a separate span should do the trick for that as well, together with the CSS direction property.

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.

3 participants