-
Notifications
You must be signed in to change notification settings - Fork 13
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
Only lead climbs #60
base: master
Are you sure you want to change the base?
Only lead climbs #60
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.
Et voilà pour les commentaires sur la partie front !
Est-ce que tu pourra mettre une capture d'écran ? ça permet d'imager la PR : )
# Conflicts: # package-lock.json # package.json
… one crag, icon were sames and false. Added cragRoute props and also added ascent_status and roping_status in backend api ascended_crag_routes
Replaced `onlyLeadClimbs` with a more flexible `filters` system across outdoor ascents, analytics, and related components. Introduced a method to dynamically generate filter parameters, updated UI elements to handle multiple filters, and adjusted API calls accordingly. Enhanced translation files to support new filter options.
…mplement filters for other users)
Voir les commentaires globaux api/app et les screenshots sur la PR api. |
Enhanced filtering capability by introducing multi-select filters for ascents, roping statuses, and climbing types. Replaced toggle buttons with form-based filters, which persist via local storage. Updated components to support these changes and refactored corresponding data handling for better user experience.
Unified and streamlined the filter handling across multiple components and APIs by replacing specific keys (like climbingType) with a generalized filters object. Simplified the structure in components and improved maintainability by leveraging watchers and emitting updates automatically.
Le front à l'air pas mal ! Mais on commence à avoir quelque chose de vraiment pas mal ! |
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.
Cette page est très redondante avec la page index.vue.
On pourrait pousser plus loin et faire un composant outdoorLogBook.vue utilisé par les deux vues.
Dis moi si tu veux que j'essaie ...
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.
je propose de faire la proposition ci dessus avant de mettre en prod. Qu'en penses tu ?
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.
En fais cette page intègre juste les tabs : (Carnet / Tick List / Projets / Analyticks / Ma carte)
et suivant ta route ça inclue soit le carnet (index.vue), soit la Tick List (tick-list.vue) soit Projets (projects.vue), etc.
Ou alors je ne comprends pas ta question ?
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.
Ok désolé je viens de comprends, oui il y a une redondance entre le carnet de croix d'un autre grimpeur et son carnet de croix
Oui une refacto va être nécessaire ! surtout que tu supprime les routes dans users côté api : )
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.
Merci encore pour le Job !
Et désolé, je suis pointilleux 😅
Encore une fois, si tu souhaite que je prenne la suite, n'hésite pas ! Ce que tu as fait est déjà énorme
Non je continue, ca ma plait bien d'être pointilleux et de ne mettre en prod que quand c'est nickel. |
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.
J'ai fais les modifs de tous tes comments sauf pour le Camelcase. Voir mes remarques et questions.
Sinon qu'est ce que tu penses de ma proposition : https://github.com/oblyk/oblyk-app/pull/60/files#r1900780822
return { | ||
showForm: false, | ||
filters: { | ||
ascent_status_list: [], |
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.
J'ai mis en snake case parce que tu l'as demandé pour le backend (oblyk/oblyk-api#9 (comment)). Et effectivement il y a forcément un des deux bouts qui n'est pas vraiment dans la norme. Pour etre plus cnofoirme la seule solution que je vois c'est de convertir le Camel en snake avant envoi de la requete comme ci-dessous. Est ce que ca te vas ?
// Fonction pour convertir camelCase en snake_case
function camelToSnake(obj) {
const newObj = {}
for (const key in obj) {
if (obj.hasOwnProperty(key)) {
const newKey = key.replace(/([A-Z])/g, '_$1').toLowerCase()
newObj[newKey] = obj[key]
}
}
return newObj
}
class LogBookOutdoorApi extends BaseApi {
// si user_id est null, il retournera le logbook de l'utilisateur actuel
stats (stats_list = {}, filters = {}, user_id = null) {
// Convertir les clés de filters en snake_case
const snakeCaseFilters = camelToSnake(filters)
return this.axios.request({
method: 'GET',
url: `${this.baseUrl}/current_users/log_books/outdoors/stats.json`,
headers: {
Authorization: this.authToken(),
HttpApiAccessToken: this.apiAccessToken
},
params: {
user_id,
filters: snakeCaseFilters,
stats_list
},
paramsSerializer: params => {
return qs.stringify(params, { arrayFormat: 'brackets', encode: false })
}
})
}
}
|
||
watch: { | ||
// Watch for changes in filters and emit automatically | ||
filters: { |
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.
On ouvre la form de filtre en cliquant sur le bouton "filtrer".
Tant qu'il est ouvert on peut changer les paramètres du filtre et on observe à chaque changement l'effet sur les stats (le watch) mais sans sauver dans le localStorage.
Quand on clique sur "Save" ca enregistre le noveau régalge de filtres dans le localStorage (le emit)
return { | ||
showForm: false, | ||
filters: { | ||
ascent_status_list: [], |
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.
Finalement c'est beaucoup trop lourd. Dans l'autre sens il faut aussi convertir tous les snakes en camelCase à la réception des données. Tu en penses quoi ?
return { | ||
showForm: false, | ||
filters: { | ||
ascent_status_list: [], |
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.
info: pour repérer ou j'avais des snakes case à remplacer j'ai voulu ajouter ' camelcase: ['error', { properties: 'always' }] ' au linter. Il a sorti 1884 errors. Donc je l'ai enlevé ... Mais ce n'est aps le seul endroit ou on a des snake cases.
return { | ||
showForm: false, | ||
filters: { | ||
ascent_status_list: [], |
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.
Alors oui ce n'est pas ultra régulier que tous soit en camelCase ^^
Grosso modo, sur ruby on rails la convention c'est le snake_case pour les variables
Sur nuxt / js c'est le cameCase, du moins c'est la convention que j'ai choisie
Donc ça fait forcément un mixte de convention.
Par exemple les données issu de l'API sont en snake_case, exemple ascent.roping_status
donc dans le formulaire d'ajout d'une croix, tu va avoir ce genre de code
data () {
return {
loadingAscent: true,
data: {
id: this.ascent.id,
roping_status: this.ascent.roping_status
}
}
}
là par exemple tu as un mixe de variable issu de l'API dans this.data, et des variable qui servent le fonctionnement du front le this.loadingAscent
Donc maintenant ce pose la question de passage d'un paramètre du fonte vers le back, comme ici
Dans la plus part des cas je travail sur le front avec les convention du front, et sur le back avec les conventions du back, donc ça va être au niveau des serviceApi que la conversion va s’effectuer.
exemple:
class LogBookOutdoorApi extends BaseApi {
stats (stats_list = {}, filters = {}, user_id = null) {
return this.axios.request({
method: 'GET',
url: `${this.baseUrl}/current_users/log_books/outdoors/stats.json`,
headers: {
Authorization: this.authToken(),
HttpApiAccessToken: this.apiAccessToken
},
params: {
user_id,
filters: {
roping_status: filters.ropingStatus,
climbing_type: filters.climbingType
},
stats_list
},
paramsSerializer: params => {
return qs.stringify(params, { arrayFormat: 'brackets', encode: false })
}
})
}
}
personnellement ça ne me dérange pas de répéter les filtres passé à l'API, ça facilite la lecture : )
et on peut appeler cette fonction comme ça :
new LogBookOutdoorApi(this.$axios, this.$auth).stats(this.statsList, this.filters)
donc par exemple pour statList on travail en camelCase dans le reste du front, il est juste naturellement transformé en en snake_case dans LogBookOutdoorApi au passage par les paramètres
|
||
watch: { | ||
// Watch for changes in filters and emit automatically | ||
filters: { |
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.
En local ça doit bien marcher, mais en prod avec des temps de réponse plus long je ne suis pas sûr que l'expérience soit aussi gratifiante.
Surtout si on ne gère pas l'annulation de la requête précédente.
Si tu laisse l'utilisateur envoyer un second filtre avant d'avoir reçu le premier, tu n'as pas la garantie de l'ordre dans lequel tu va avoir les réponses, tu va peut-être recevoir la réponse du deuxième filter, puis du premier, et tu te retrouve avec les croix du premier filtre avec l'affichage du second filtre.
Il fois soit annuler la requête précédente, soit bloquer le formulaire temps que tu n'as pas reçu la réponse du premier, soit filtrer uniquement quand tu clic sur (filtrer)
Je pense que le plus compréhensible, performant et simple c'est la troisième solution.
Un seul bouton (filter) qui envoie la requête et enregistre le filtrer
J'ai refactoré en Camelcase et tout a la racine, plus diverses autres remarques appliquées. Pas certain que tout soit ok. Dis moi si il reste des pb de cohérence. |
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.
du package-lock.json ne devait pas changer
Ça a l'air pas mal ! |
Evolution liée au ticket "croix en tete et/ou en moulinette #59" pour les deux points.
1 - Permet de n'afficher que les croix en tête avec un selecteur pour ceux qui veulent voir aussi les moulinettes.
2 - Ne compte pas les répétitions d'une même voie dans les croix
Corrige également : les grandes voies sont comptées comme croix dans le niveau max parmis les sections réalisées dans l'ascent (auparavant elles comptaient dans le niveau min de toutes les sections de la crag_route)
A combiner avec la pull request sur oblyk-api