-
-
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
Support for icon_map #753
Support for icon_map #753
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 this contribution. It's looking great so far. I've added a few comments and suggestions. Can you please apply them and let me know when this is ready for another review? Thanks again!
src/types.ts
Outdated
export type Conditions = 'clear-night' | 'cloudy' | 'fog' | 'hail' | 'lightning' | 'lightning-rainy' | 'partlycloudy' | 'pouring' | 'rainy' | 'snowy' | 'snowy-rainy' | 'sunny' | 'windy' | 'windy-variant' | 'exceptional' | ||
export type IconMap = Partial<Record<Conditions, string>> |
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.
export type Conditions = 'clear-night' | 'cloudy' | 'fog' | 'hail' | 'lightning' | 'lightning-rainy' | 'partlycloudy' | 'pouring' | 'rainy' | 'snowy' | 'snowy-rainy' | 'sunny' | 'windy' | 'windy-variant' | 'exceptional' | |
export type IconMap = Partial<Record<Conditions, string>> | |
export type Condition = 'clear-night' | 'cloudy' | 'fog' | 'hail' | 'lightning' | 'lightning-rainy' | 'partlycloudy' | 'pouring' | 'rainy' | 'snowy' | 'snowy-rainy' | 'sunny' | 'windy' | 'windy-variant' | 'exceptional'; | |
type PerConditionConfig<TValue> = Partial<Record<Condition, TValue>>; | |
export type IconMap = PerRecordConfig<string>; |
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.
And then replace the ColorConfig
interface below with:
export type ColorConfig extends PerConditionConfig<ColorDefinition>;
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.
And edit the type of condition
in ConditionSpan
and ForecastSegment
to Condition
instead of string
.
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.
I couldn't make it work with export type ColorConfig extends PerConditionConfig<ColorDefinition>;
. I tried export interface ColorConfig extends PerConditionConfig<ColorDefinition> {};
but then I got a bunch of type errors every time it iterates over ColorConfig
key/value because it thinks value
is unknown
.
Example:
I guess I can fix it with a bunch of as ColorDefinition
, but the forced cast feels wrong to me.
I'm not sure if what I did is equivalent tbh. Let me know what you think.
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.
Oh, my bad - I originally had that comment suggesting an interface, but changed to a type and forgot to adjust the syntax. You got it right.
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 adjusting. Looks great.
What problem does this PR solve?
Adds support for use custom icons for the weather conditions. This will help the use case where the original icons are too small (or too similar) to be read from a distance, or just be able to use a different icon set for aesthetic reasons.
Proposed solution
Adds a config option
icon_map
. This YAML dictionary will allow to map each weather condition to an icon to use. If an icon is not present in the map, the default (i.e. themdi:*
icon) will be used.For example, this config will produce this result: