nextcloud / weather

⛅️ Weather app for Nextcloud
GNU Affero General Public License v3.0
47 stars 27 forks source link

Vue Migration - fixes #80 #92

Open berdosi opened 3 years ago

berdosi commented 3 years ago

This is quite a mouthful, for a full rewrite of the frontend template (API calls were kept pretty much as-is)

It builds with npm install && npm build. The bundle is around 1MB, which seems large - but comparable to all the other Nextcloud applications.

Also, I've added a blur to the content panels, which could work around #90 (only works on WebKit):

screenshot-201021-180722

e-alfred commented 3 years ago

This looks very nice, thanks for your hard work! Do you think it is ready to be merged already?

nerzhul commented 3 years ago

i like the new additions you did too, didn't reviewed the code

berdosi commented 3 years ago

Thank you ! I just kept fixing issues since the fcda994 commit, and there were no new ones on my end.

What I didn't pay much attention to is what happens when adding / removing cities, and some of the current behavior may be considered a regression: We might be (adding / removing) a city which (is / is not) the home city and (is / is not) currently selected, and as a result there are (0 / 1 / more) cities. How should this effect the selected / home city?

e-alfred commented 3 years ago

This looks quite awesome! Do you think we should wait and include it in a possible release for Nextcloud 20 or make a release for Nextcloud 20 anyway with the legacy Angular.js interface?

berdosi commented 3 years ago

This PR doesn't depend on Nextcloud 20 APIs – it may be released whenever it's considered ready.

Nextcloud 20 may warrant a rewrite on the backend, though: it comes with its built-in Dashboard including weather data from met.no's API. It makes sense to investigate piggybacking on it.

e-alfred commented 3 years ago

@berdosi The old dashboard code should be removed because it is not useful anymore.

Do you have an idea on how to integrate the best way with the Weather Status app by @eneiluj? I am just thinking that maybe it shouldn't send the user to an external page but rather automatically add the preferred location to the Weather app if it is installed. Maybe a separate menu in the settings could allow the user to set their preferred option.

berdosi commented 3 years ago

Indeed it should be – unless someone's faster, I may open a PR for that soonish.

I know that currently the link in the Dashboard points to Windy.com, for the current coordinates. An idea would be to implement #85 (geolocation), so that it could be linking into Weather app.

I see two ways of changing the link in the widget, in case the app is installed - and both are ugly:

  1. either the Widget would be looking for the App, and change its behavior accordingly – which I don't see happening.
  2. maybe, Weather app could use Util::addScript() (just like in a way Unsplash uses addStyle()), and patch up the dashboard widget's markup, if it exists. I don't like this.

There are probably more civilized ways of apps interopping with one another – I assume the Widget could expose an interface to let its forecast provider be changed by the apps (which would be similar to 1) above, but has a way better PR).

berdosi commented 3 years ago

Whoops, this has been closed in error.

e-alfred commented 3 years ago

@berdosi Do you think this is ready for merging?

berdosi commented 3 years ago

@e-alfred yes, I think it is: I've upgraded the dependencies and solved the pending merge conflicts. I am somewhat unsure whether 5c2ae40 commit can be reverted: app.js and angular.min.js are redundant, but putting them back was the easiest way to fix the conflict.

e-alfred commented 3 years ago

@berdosi It might be necessary to rebase on the latest master to get the most recent changes from there.

berdosi commented 3 years ago

@e-alfred Thanks for the tip – I like what I'm seeing now. (I ended up force-pushing due to a failing DCO check.)

almereyda commented 3 years ago

It appears this PR is mergeable. Is there anything we can do as the community to support the maintainers in reviewing this PR, in so it can be merged?