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

Only lead climbs #60

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

Only lead climbs #60

wants to merge 32 commits into from

Conversation

pectum83
Copy link
Contributor

@pectum83 pectum83 commented Sep 30, 2023

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

@lucien-chastan lucien-chastan added enhancement New feature or request outdoor tag for the outdoor part labels Nov 19, 2024
Copy link
Contributor

@lucien-chastan lucien-chastan left a 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 : )

components/logBooks/outdoors/LogBookList.vue Outdated Show resolved Hide resolved
package-lock.json Outdated Show resolved Hide resolved
pages/home/ascents/outdoor/analytiks.vue Outdated Show resolved Hide resolved
pages/home/ascents/outdoor/analytiks.vue Outdated Show resolved Hide resolved
pages/home/ascents/outdoor/index.vue Outdated Show resolved Hide resolved
services/oblyk-api/LogBookOutdoorApi.js Outdated Show resolved Hide resolved
services/oblyk-api/LogBookOutdoorApi.js Outdated Show resolved Hide resolved
# 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.
@pectum83
Copy link
Contributor Author

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.
@lucien-chastan
Copy link
Contributor

Le front à l'air pas mal !
Je te laisse voir les commentaires que j'ai fait sur oblyk/oblyk-api#9 avant de relire cette PR, ça va sûrement encore un peu impacter le code du front

Mais on commence à avoir quelque chose de vraiment pas mal !
Merci encore de ta contribution !!

Copy link
Contributor Author

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 ...

Copy link
Contributor Author

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 ?

Copy link
Contributor

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 ?

Copy link
Contributor

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 : )

Copy link
Contributor

@lucien-chastan lucien-chastan left a 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

components/forms/AscentStatusInput.vue Outdated Show resolved Hide resolved
components/forms/ClimbingTypeInput.vue Show resolved Hide resolved
components/forms/ClimbingTypeInput.vue Outdated Show resolved Hide resolved
components/forms/RopingStatusInput.vue Outdated Show resolved Hide resolved
components/logBooks/outdoors/AscentFiltersForm.vue Outdated Show resolved Hide resolved
lang/en-US.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
pages/climbers/_userName/ascents/outdoor.vue Show resolved Hide resolved
pages/home/ascents/outdoor/analytiks.vue Show resolved Hide resolved
services/oblyk-api/LogBookOutdoorApi.js Outdated Show resolved Hide resolved
@pectum83
Copy link
Contributor Author

pectum83 commented Jan 7, 2025

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.

Copy link
Contributor Author

@pectum83 pectum83 left a 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: [],
Copy link
Contributor Author

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

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: [],
Copy link
Contributor Author

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: [],
Copy link
Contributor Author

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.

components/forms/ClimbingTypeInput.vue Show resolved Hide resolved
return {
showForm: false,
filters: {
ascent_status_list: [],
Copy link
Contributor

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: {
Copy link
Contributor

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

lang/en-US.js Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
pages/climbers/_userName/ascents/outdoor.vue Outdated Show resolved Hide resolved
@pectum83
Copy link
Contributor Author

pectum83 commented Jan 8, 2025

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.

Copy link
Contributor

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

@lucien-chastan
Copy link
Contributor

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.

Ça a l'air pas mal !
Il faut que je l'installe en local maintenant voir s'il y reste des choses à l'usage : )
Et il y a un conflit à gérer avec le master

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request outdoor tag for the outdoor part
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants