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

Vue Migration - fixes #80 #92

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

Conversation

berdosi
Copy link
Contributor

@berdosi berdosi commented Oct 21, 2020

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

  • Uses @nextcloud/vue components, giving an uniform feel
  • Responsive behavior is reworked (there is no playing around with font sizes, just columns being hidden / shown)
  • CSS uses classes instead of IDs.
  • Home city is set from the city list.

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
Copy link
Collaborator

e-alfred commented Nov 1, 2020

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

@nerzhul
Copy link
Collaborator

nerzhul commented Nov 3, 2020

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

@berdosi
Copy link
Contributor Author

berdosi commented Nov 8, 2020

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
Copy link
Collaborator

e-alfred commented Nov 9, 2020

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
Copy link
Contributor Author

berdosi commented Nov 9, 2020

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
Copy link
Collaborator

e-alfred commented Nov 9, 2020

@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
Copy link
Contributor Author

berdosi commented Nov 10, 2020

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 berdosi closed this Nov 10, 2020
@berdosi
Copy link
Contributor Author

berdosi commented Nov 10, 2020

Whoops, this has been closed in error.

@berdosi berdosi reopened this Nov 10, 2020
@e-alfred e-alfred mentioned this pull request Dec 24, 2020
@e-alfred
Copy link
Collaborator

@berdosi Do you think this is ready for merging?

@berdosi
Copy link
Contributor Author

berdosi commented Dec 28, 2020

@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
Copy link
Collaborator

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

@berdosi
Copy link
Contributor Author

berdosi commented Dec 29, 2020

@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
Copy link

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?

berdosi and others added 2 commits April 11, 2021 12:44
Copy link
Member

@CarlSchwan CarlSchwan left a comment

Choose a reason for hiding this comment

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

Code looks good, mostly nitpicks

  • Use async/await
  • Move store definition to another file
  • Forgot to translate a few strings

src/App.vue Outdated Show resolved Hide resolved
src/App.vue Outdated Show resolved Hide resolved
Comment on lines +127 to +289
context.commit('setApiKeyNeeded', false)
} else if (reason.status === 401) {
const error = t(
'weather',
'Your OpenWeatherMap API key is invalid. Contact your administrator to configure a valid API key in Additional Settings of the Administration')
showError(error)

context.commit('setCityLoadError', error)
context.commit('setApiKeyNeeded', true)
} else {
const error = context.getters.fatalError

showError(error)

logger.error('Error getting weather information for city', { reason })
context.commit('setCityLoadError', error)
context.commit('setApiKeyNeeded', false)
}

})
},
deleteCity(context, cityId) {
axios.post(generateUrl('/apps/weather/city/delete'), { id: cityId })
.then((response) => {
if (response.data != null && !!response.data.deleted) {
context.commit('removeCity', cityId)
// If current city is the removed city, close it
if (context.state.selectedCityId === cityId) {
context.commit('unsetSelected')
// if there are still cities on the list, and the selected was deleted, select the home city.
// if the home city was deleted, select the first one instead
if (context.state.allCities.cities.length > 0) {
context.dispatch(
'loadCity',
cityId !== context.state.allCities.home
? context.state.allCities.home
: context.state.allCities.cities[0].id)
}
}
} else {
alert(t('weather', 'Failed to remove city. Please contact your administrator'))
}
}).catch((reason) => {
showError(context.getters.fatalError)
logger.error('Error deleting city', { reason })
})
},
setHome(context, cityId) {
axios.post(generateUrl('/apps/weather/settings/home/set'), { city: cityId })
.then(response => {
if (response.data != null && !!response.data.set) {
context.commit('setHome', cityId)
} else {
showError(t('weather', 'Failed to set home. Please contact your administrator'))
}
})
.catch((reason) => {
logger.error('Error setting home city', { reason })
showError(context.getters.fatalError)
})
},
selectCity(context, cityId) {
context.dispatch('loadCity', cityId)
.then(() => {
context.commit('setSelected', cityId)
})
},
loadMetric(context) {
axios.get(generateUrl('/apps/weather/settings/metric/get'))
.then((result) => {
if (result.data.metric) {
context.commit('setMetric', result.data.metric)
context.commit('setMetricRepresentation', mapMetric(result.data.metric))
}
}).catch((reason) => {
logger.error('Error setting metric', { reason })
showError(context.getters.fatalError)
})
},
modifyMetric(context, metric) {
axios.post(generateUrl('/apps/weather/settings/metric/set'), { metric })
.then((result) => {
if (result.data != null && !!(result.data.set)) {
context.commit('setMetric', metric)
context.commit('setMetricRepresentation', mapMetric(metric))
context.dispatch('loadCity', context.state.selectedCityId)
} else {
showError(t('weather', 'Failed to set metric. Please contact your administrator'))
}
}).catch(
(reason) => {
if (reason.status === 404) {
showError(t('weather', 'This metric is not known.'))
} else {
logger.error('Error setting metric', { reason })
showError(context.getters.fatalError)
}
})
},
},
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
addCity(context, newCityName) {
axios.post(generateUrl('/apps/weather/city/add'), { name: newCityName })
.then((response) => {
showSuccess(`City added: ${newCityName}`)
if (response.data.id) {
// response contains the id, but not the name
context.commit('addCity', { name: newCityName, id: response.data.id })
}
// otherwise one may send a new API call : context.dispatch('loadCities')
})
.catch((reason) => {
logger.error('Error adding city', { reason })
showError(context.getters.fatalError)
})
},
loadCities(context) {
axios.get(generateUrl('/apps/weather/city/getall'))
.then((response) => {
const responseData = response.data
if (responseData.home) {
context.commit('setHome', responseData.home)
}
if (responseData.cities) {
context.commit('setCities', responseData.cities)
}
if (!context.state.loaded) {
if (responseData.home && !!responseData.cities.find(city => city.id === responseData.home)) {
// there is a home city set, and it hasnn't been removed before -> load it
context.dispatch('loadCity', responseData.home)
.then(() => { context.commit('setLoaded', true) })
} else if (responseData.cities && responseData.cities.length > 0) {
// load the first city, if no home city is set
context.dispatch('loadCity', responseData.cities[0].id)
}
}
})
.catch(reason => {
showError(context.getters.fatalError)
logger.error('Cannot load cities', { reason })
})
},
loadCity(context, cityId) {
const cityToLoad = context.state.allCities.cities.find(city => city.id === cityId)
if (!cityToLoad) return
axios.get(
generateUrl('/apps/weather/weather/get'),
{ params: { name: cityToLoad.name } })
.then((response) => {
const currentCityData = response.data
currentCityData.wind.desc = windMapper(currentCityData.wind.deg)
currentCityData.image = imageMapper(currentCityData.weather[0].main)
context.commit('setCurrentCity', currentCityData)
context.commit('setSelected', cityToLoad.id)
})
.catch((reason) => {
showError(`Cannot load city: ${reason}`)
if (reason.status === 404) {
const error = t('weather', 'No city with this name found.')
showError(error)
context.commit('setCityLoadError', error)
context.commit('setApiKeyNeeded', false)
} else if (reason.status === 401) {
const error = t(
'weather',
'Your OpenWeatherMap API key is invalid. Contact your administrator to configure a valid API key in Additional Settings of the Administration')
showError(error)
context.commit('setCityLoadError', error)
context.commit('setApiKeyNeeded', true)
} else {
const error = context.getters.fatalError
showError(error)
logger.error('Error getting weather information for city', { reason })
context.commit('setCityLoadError', error)
context.commit('setApiKeyNeeded', false)
}
})
},
deleteCity(context, cityId) {
axios.post(generateUrl('/apps/weather/city/delete'), { id: cityId })
.then((response) => {
if (response.data != null && !!response.data.deleted) {
context.commit('removeCity', cityId)
// If current city is the removed city, close it
if (context.state.selectedCityId === cityId) {
context.commit('unsetSelected')
// if there are still cities on the list, and the selected was deleted, select the home city.
// if the home city was deleted, select the first one instead
if (context.state.allCities.cities.length > 0) {
context.dispatch(
'loadCity',
cityId !== context.state.allCities.home
? context.state.allCities.home
: context.state.allCities.cities[0].id)
}
}
} else {
alert(t('weather', 'Failed to remove city. Please contact your administrator'))
}
}).catch((reason) => {
showError(context.getters.fatalError)
logger.error('Error deleting city', { reason })
})
},
setHome(context, cityId) {
axios.post(generateUrl('/apps/weather/settings/home/set'), { city: cityId })
.then(response => {
if (response.data != null && !!response.data.set) {
context.commit('setHome', cityId)
} else {
showError(t('weather', 'Failed to set home. Please contact your administrator'))
}
})
.catch((reason) => {
logger.error('Error setting home city', { reason })
showError(context.getters.fatalError)
})
},
selectCity(context, cityId) {
context.dispatch('loadCity', cityId)
.then(() => {
context.commit('setSelected', cityId)
})
},
loadMetric(context) {
axios.get(generateUrl('/apps/weather/settings/metric/get'))
.then((result) => {
if (result.data.metric) {
context.commit('setMetric', result.data.metric)
context.commit('setMetricRepresentation', mapMetric(result.data.metric))
}
}).catch((reason) => {
logger.error('Error setting metric', { reason })
showError(context.getters.fatalError)
})
},
modifyMetric(context, metric) {
axios.post(generateUrl('/apps/weather/settings/metric/set'), { metric })
.then((result) => {
if (result.data != null && !!(result.data.set)) {
context.commit('setMetric', metric)
context.commit('setMetricRepresentation', mapMetric(metric))
context.dispatch('loadCity', context.state.selectedCityId)
} else {
showError(t('weather', 'Failed to set metric. Please contact your administrator'))
}
}).catch(
(reason) => {
if (reason.status === 404) {
showError(t('weather', 'This metric is not known.'))
} else {
logger.error('Error setting metric', { reason })
showError(context.getters.fatalError)
}
})
},
},
async addCity(context, newCityName) {
try {
const { data } = await axios.post(generateUrl('/apps/weather/city/add'), { name: newCityName })
showSuccess(`City added: ${newCityName}`)
if (data.id) {
// response contains the id, but not the name
context.commit('addCity', { name: newCityName, id: response.data.id })
}
// otherwise one may send a new API call : context.dispatch('loadCities')
} catch (reason) {
logger.error('Error adding city', { reason })
showError(context.getters.fatalError)
}
},
async loadCities(context) {
const { data } = await axios.get(generateUrl('/apps/weather/city/getall'))
try {
if (data.home) {
context.commit('setHome', data.home)
}
if (data.cities) {
context.commit('setCities', data.cities)
}
if (!context.state.loaded) {
if (data.home && !!data.cities.find(city => city.id === data.home)) {
// there is a home city set, and it hasnn't been removed before -> load it
context.dispatch('loadCity', responseData.home)
.then(() => { context.commit('setLoaded', true) })
} else if (data.cities && data.cities.length > 0) {
// load the first city, if no home city is set
context.dispatch('loadCity', data.cities[0].id)
}
}
} catch (reason) {
showError(context.getters.fatalError)
logger.error('Cannot load cities', { reason })
}
},
async loadCity(context, cityId) {
const cityToLoad = context.state.allCities.cities.find(city => city.id === cityId)
if (!cityToLoad) {
return
}
try {
const { data } = await axios.get(generateUrl('/apps/weather/weather/get'), {
params: { name: cityToLoad.name }
})
const currentCityData = data
currentCityData.wind.desc = windMapper(currentCityData.wind.deg)
currentCityData.image = imageMapper(currentCityData.weather[0].main)
context.commit('setCurrentCity', currentCityData)
context.commit('setSelected', cityToLoad.id)
} catch (reason) {
showError(`Cannot load city: ${reason}`)
if (reason.status === 404) {
const error = t('weather', 'No city with this name found.')
showError(error)
context.commit('setCityLoadError', error)
context.commit('setApiKeyNeeded', false)
} else if (reason.status === 401) {
const error = t(
'weather',
'Your OpenWeatherMap API key is invalid. Contact your administrator to configure a valid API key in Additional Settings of the Administration')
showError(error)
context.commit('setCityLoadError', error)
context.commit('setApiKeyNeeded', true)
} else {
const error = context.getters.fatalError
showError(error)
logger.error('Error getting weather information for city', { reason })
context.commit('setCityLoadError', error)
context.commit('setApiKeyNeeded', false)
}
}
},
async deleteCity(context, cityId) {
try {
const { data } = await axios.post(generateUrl('/apps/weather/city/delete'), { id: cityId })
if (data != null && !!data.deleted) {
context.commit('removeCity', cityId)
// If current city is the removed city, close it
if (context.state.selectedCityId === cityId) {
context.commit('unsetSelected')
// if there are still cities on the list, and the selected was deleted, select the home city.
// if the home city was deleted, select the first one instead
if (context.state.allCities.cities.length > 0) {
context.dispatch(
'loadCity',
cityId !== context.state.allCities.home
? context.state.allCities.home
: context.state.allCities.cities[0].id)
}
}
} else {
alert(t('weather', 'Failed to remove city. Please contact your administrator'))
}
} catch (reason) {
showError(context.getters.fatalError)
logger.error('Error deleting city', { reason })
}
},
async setHome(context, cityId) {
try {
const { data } = await axios.post(generateUrl('/apps/weather/settings/home/set'), { city: cityId })
if (data != null && !!data.set) {
context.commit('setHome', cityId)
} else {
showError(t('weather', 'Failed to set home. Please contact your administrator'))
}
})
} catch (reason) {
logger.error('Error setting home city', { reason })
showError(context.getters.fatalError)
}
},
async selectCity(context, cityId) {
await context.dispatch('loadCity', cityId)
context.commit('setSelected', cityId)
},
async loadMetric(context) {
try {
const { data } = await axios.get(generateUrl('/apps/weather/settings/metric/get'))
if (data.metric) {
context.commit('setMetric', data.metric)
context.commit('setMetricRepresentation', mapMetric(data.metric))
}
} catch (reason) {
logger.error('Error setting metric', { reason })
showError(context.getters.fatalError)
}
},
async modifyMetric(context, metric) {
try {
const { data } = axios.post(generateUrl('/apps/weather/settings/metric/set'), { metric })
if (data != null && !!(data.set)) {
context.commit('setMetric', metric)
context.commit('setMetricRepresentation', mapMetric(metric))
context.dispatch('loadCity', context.state.selectedCityId)
} else {
showError(t('weather', 'Failed to set metric. Please contact your administrator'))
}
} catch (reason) {
if (reason.status === 404) {
showError(t('weather', 'This metric is not known.'))
} else {
logger.error('Error setting metric', { reason })
showError(context.getters.fatalError)
}
}
},
},


Vue.use(Vuex)

const store = new Vuex.Store({
Copy link
Member

Choose a reason for hiding this comment

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

store definition should be moved to another file

nerzhul and others added 2 commits August 25, 2022 12:17
Co-authored-by: Carl Schwan <[email protected]>
Co-authored-by: Carl Schwan <[email protected]>
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.

5 participants