Skip to content

[WeatherStatus] Support more weather codes, change icons, fix style#48353

Merged
julien-nc merged 6 commits intomasterfrom
enh/30551/weather-status-support-more-codes
Sep 25, 2024
Merged

[WeatherStatus] Support more weather codes, change icons, fix style#48353
julien-nc merged 6 commits intomasterfrom
enh/30551/weather-status-support-more-codes

Conversation

@julien-nc
Copy link
Member

@julien-nc julien-nc commented Sep 25, 2024 •
edited by susnux
Loading

This branch is based on the one of #47702.

  • Use icons provided by met.no at https://github.com/metno/weathericons
  • Support more weather codes (a few snow ones)
  • Fix location icon with dark theme
  • Only use icons from @mdi/svg
  • Fix alignment and color issues
  • Make a difference between when no location has been set and when the weather code is unknown/unsupported
Before After
image image
image image

@julien-nc julien-nc added this to the Nextcloud 30.0.1 milestone Sep 25, 2024
@julien-nc julien-nc requested review from a team, Pytal, artonge, jancborchardt and susnux and removed request for a team September 25, 2024 11:18
@julien-nc julien-nc force-pushed the enh/30551/weather-status-support-more-codes branch from bf8943e to 64fae5b Compare September 25, 2024 11:20
susnux
susnux reviewed Sep 25, 2024
View reviewed changes
apps/weather_status/src/App.vue Outdated
min-width: 44px !important;
min-height: 44px !important;
.weather-image {
width: 28px;
Copy link
Contributor

@susnux susnux Sep 25, 2024

Choose a reason for hiding this comment

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

Magic value? Why 28px?
Our default icons are 16px (which is ~20px MDI icon size)

Copy link
Member Author

@julien-nc julien-nc Sep 25, 2024

Choose a reason for hiding this comment

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

I don't mind using 20px. It's just less visible.
I just raised the size as much as possible to make it easier to see/identify the icon. These weather icons are slightly more important than classic ones IMO.

Copy link
Contributor

@susnux susnux Sep 25, 2024

Choose a reason for hiding this comment

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

I agree, but then maybe:

    width: calc(var(--default-clickable-area) - 2 * var(--default-grid-baseline));

susnux
susnux reviewed Sep 25, 2024
View reviewed changes
susnux
susnux requested changes Sep 25, 2024
View reviewed changes
Copy link
Contributor

@susnux susnux left a comment

Choose a reason for hiding this comment

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

License information is important

@julien-nc julien-nc requested a review from susnux September 25, 2024 14:24
susnux
susnux reviewed Sep 25, 2024
View reviewed changes
susnux
susnux reviewed Sep 25, 2024
View reviewed changes
susnux
susnux reviewed Sep 25, 2024
View reviewed changes
@susnux
Copy link
Contributor

susnux commented Sep 25, 2024

License information is important

Thank you for fixing, LicenseRef-freepikLicense is now unused so you need to remove that from /Licenses directory :)

@julien-nc julien-nc force-pushed the enh/30551/weather-status-support-more-codes branch 2 times, most recently from 5f0b0de to b487b8b Compare September 25, 2024 14:50
@julien-nc julien-nc requested a review from susnux September 25, 2024 14:50
susnux
susnux approved these changes Sep 25, 2024
View reviewed changes
apps/weather_status/src/App.vue Outdated
min-width: 44px !important;
min-height: 44px !important;
.weather-image {
width: 20px;
Copy link
Contributor

@susnux susnux Sep 25, 2024

Choose a reason for hiding this comment

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

If you prefer bigger icons:

Suggested change
width: 20px;
width: calc(var(--default-clickable-area) - 2 * var(--default-grid-baseline));

@julien-nc julien-nc force-pushed the enh/30551/weather-status-support-more-codes branch from b487b8b to 1b7ae47 Compare September 25, 2024 15:08
XxTysweezyxX and others added 4 commits September 25, 2024 17:24
@XxTysweezyxX @julien-nc
Fix #30551: Added snowfall option to weather status
1826ba2 
Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
@julien-nc
feat(weatherstatus): support all weather codes from api.met.no, use t…
bfa1c7e 
…he icons provided by met.no, cleanup UI

Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
@julien-nc
fix(weatherstatus): make a difference between 'unknown weather code' …
c96bde7 
…and 'no weather info'

Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
@julien-nc
fix(weatherstatus): fix location marker icon with dark theme, only us…
966a0a9 
…e icons from @mdi/svg

Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
@julien-nc julien-nc force-pushed the enh/30551/weather-status-support-more-codes branch from 1b7ae47 to e73c03c Compare September 25, 2024 15:24
@julien-nc

This comment was marked as outdated.

Sign in to view
@julien-nc julien-nc mentioned this pull request Sep 25, 2024
Closed
5 tasks
@julien-nc
fix(weatherstatus): adjustments from review comments
efcc3e5 
Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
@julien-nc julien-nc force-pushed the enh/30551/weather-status-support-more-codes branch from e73c03c to efcc3e5 Compare September 25, 2024 15:41
@julien-nc
Copy link
Member Author

julien-nc commented Sep 25, 2024

/compile

@nextcloud-command
chore(assets): Recompile assets
45a3845 
Signed-off-by: nextcloud-command <nextcloud-command@users.noreply.github.com>
@julien-nc julien-nc merged commit b5da085 into master Sep 25, 2024
@julien-nc julien-nc deleted the enh/30551/weather-status-support-more-codes branch September 25, 2024 16:25
@provokateurin provokateurin mentioned this pull request Sep 25, 2024
Closed
@jancborchardt
Copy link
Member

jancborchardt commented Oct 22, 2024

@julien-nc very nice – just one detail: The previous icons worked a bit nicer with the style of Nextcloud, is there a specific reason they got replaced? If not I think it would be good to keep using the previous ones, if that’s ok with you? :)

@skjnldsv skjnldsv mentioned this pull request Jan 7, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@susnux susnux susnux approved these changes

@artonge artonge artonge approved these changes

@jancborchardt jancborchardt Awaiting requested review from jancborchardt

@Pytal Pytal Awaiting requested review from Pytal Pytal is a code owner automatically assigned from nextcloud/server-frontend

Assignees

No one assigned

Labels

3. to review Waiting for reviews enhancement feature: dashboard

Projects

None yet

Milestone

Nextcloud 30.0.1

Development

Successfully merging this pull request may close these issues.

[weather_status] Snowfall

6 participants

@julien-nc @susnux @jancborchardt @artonge @XxTysweezyxX @nextcloud-command