Skip to content

Conversation

@vwegert
Copy link

@vwegert vwegert commented Jan 30, 2022

suggestion for issue #54

Copy link
Owner

@nkl-kst nkl-kst left a comment

Choose a reason for hiding this comment

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

@vwegert Thanks for your PR, I think these config options should be available in the module. Please let me know what you think about my review comments, thanks in advance.

{% if windDirKey %}
{{ windDirKey | translate }}
<i class="wi wi-wind from-{{data.dd}}-deg fa-fw"></i>
{% if config.showCurrentIcon or config.showCurrentTemperature or config.showCurrentText or config.showCurrentWindspeed %}
Copy link
Owner

Choose a reason for hiding this comment

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

I think we can avoid this check because an empty div won't use any space.

Copy link
Author

Choose a reason for hiding this comment

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

I believe that would depend on whatever nefarious CSS stuff is assigned to the class "current". Personally, I prefer not to generate content that is known to be superfluous, but that's certainly not a fixed rule...

<i class="wi wi-wind from-{{data.dd}}-deg fa-fw"></i>
{% if config.showCurrentIcon or config.showCurrentTemperature or config.showCurrentText or config.showCurrentWindspeed %}
<div class="current">
{% if config.showCurrentIcon or config.showCurrentTemperature %}
Copy link
Owner

Choose a reason for hiding this comment

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

Same here, I think this check isn't necessary.

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.

2 participants