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

Use SearchComponent on bouquets & données page #584

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

at-github
Copy link
Contributor

Copy link

netlify bot commented Nov 15, 2024

Deploy Preview for ecospheres ready!

Name Link
🔨 Latest commit 59df6b9
🔍 Latest deploy log https://app.netlify.com/sites/ecospheres/deploys/67489f5d3b5dc7000716afd5
😎 Deploy Preview https://deploy-preview-584--ecospheres.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Nov 15, 2024

Deploy Preview for meteo-france ready!

Name Link
🔨 Latest commit 59df6b9
🔍 Latest deploy log https://app.netlify.com/sites/meteo-france/deploys/67489f5da038fc000818fb3f
😎 Deploy Preview https://deploy-preview-584--meteo-france.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@streino
Copy link
Contributor

streino commented Nov 15, 2024

@at-github Le contenu de PR a l'air beaucoup plus conséquent que sa description. Tu es sûr que c'est bon ?

@abulte
Copy link
Contributor

abulte commented Nov 18, 2024

J'ai l'impression que la base branch devrait être celle de la home.

@at-github
Copy link
Contributor Author

at-github commented Nov 18, 2024

@at-github Le contenu de PR a l'air beaucoup plus conséquent que sa description. Tu es sûr que c'est bon ?

Elle comprend toute la branche de la home, je mets en draft

@at-github at-github marked this pull request as draft November 18, 2024 08:12
@abulte
Copy link
Contributor

abulte commented Nov 18, 2024

@at-github tu peux juste switcher la base branch de la PR, non ?

@at-github
Copy link
Contributor Author

@at-github tu peux juste switcher la base branch de la PR, non ?

J'ai rebase sur main après merge de la home

@at-github at-github marked this pull request as ready for review November 18, 2024 10:07
src/views/datasets/DatasetsListView.vue Outdated Show resolved Hide resolved
src/views/bouquets/BouquetsListView.vue Outdated Show resolved Hide resolved
src/components/SearchComponent.vue Outdated Show resolved Hide resolved
src/components/SearchComponent.vue Outdated Show resolved Hide resolved
@abulte
Copy link
Contributor

abulte commented Nov 19, 2024

Problème de focus natif navigateur (Safari et Chrome)

Capture d’écran 2024-11-19 à 09 20 34 Capture d’écran 2024-11-19 à 09 15 10

@abulte
Copy link
Contributor

abulte commented Nov 19, 2024

Comportement quand on appuie sur Entrée dans le champ de recherche :

Avant

  • On vidait le champ en lançant la recherche en appuyant sur Entrée dans le header (relativement satisfaisant car on navigue potentiellement vers une nouvelle page et on n'a pas envie de gérer la synchro entre le header et les autres champs de recherche)
  • Dans les autres champs de recherche, Entrée n'avait pas d'effet

Maintenant

  • Sur Ecosphères, on vide le champ à l'appui sur Entrée pour tous les champs : perturbant quand on est pas dans le header
  • Sur Météo, on vide tous les champs sauf le header 🤔

=> revenir au comportement existant me parait le mieux, sauf si meilleure idée.

@at-github
Copy link
Contributor Author

at-github commented Nov 20, 2024

Problème de focus natif navigateur (Safari et Chrome)

Je n'arrive pas à reproduire sur chrome (linux), j'espère qu'il ne me faut pas un mac quand même

@abulte
Copy link
Contributor

abulte commented Nov 20, 2024

Problème de focus natif navigateur (Safari et Chrome)

Je n'arrive pas à reproduire sur chrome (linux), j'espère qu'il ne me faut pas un mac quand même

Ca suit l'input

Capture d’écran 2024-11-20 à 15 18 40 Capture d’écran 2024-11-20 à 15 19 14

@narduin
Copy link
Contributor

narduin commented Nov 20, 2024

Problème de focus natif navigateur (Safari et Chrome)

Je n'arrive pas à reproduire sur chrome (linux), j'espère qu'il ne me faut pas un mac quand même

Ca suit l'input

Ça vient de DsfrSearchBar qui met le focus sur l'input comme prévu.
Sauf qu'avec le comportement actuel, le bouton ne sert à rien. D'où son positionnement anormal à gauche.

Pour harmoniser le focus:

.fr-search-bar:focus-within {
  outline: 2px solid #0a76f6;
  outline-offset: -2px;
}

.fr-search-bar input:focus {
  outline: none;
}

Il y a un autre problème: avec DsfrSearchBar, impossible de se passer du bouton ou de le déplacer dans le DOM. Ainsi, quand on navigue au clavier, on a un ordre de tabulation incohérent.

Si on garde le comportement actuel (recherche à la volée sans attendre de submit), il ne faut pas utiliser DsfrSearchBar mais un DsfrInput classique.

@at-github
Copy link
Contributor Author

Problème de focus natif navigateur (Safari et Chrome)

Je n'arrive pas à reproduire sur chrome (linux), j'espère qu'il ne me faut pas un mac quand même

Ca suit l'input

Ça vient de DsfrSearchBar qui met le focus sur l'input comme prévu. Sauf qu'avec le comportement actuel, le bouton ne sert à rien. D'où son positionnement anormal à gauche.

Pour harmoniser le focus:

.fr-search-bar:focus-within {
  outline: 2px solid #0a76f6;
  outline-offset: -2px;
}

.fr-search-bar input:focus {
  outline: none;
}

Il y a un autre problème: avec DsfrSearchBar, impossible de se passer du bouton ou de le déplacer dans le DOM. Ainsi, quand on navigue au clavier, on a un ordre de tabulation incohérent.

Si on garde le comportement actuel (recherche à la volée sans attendre de submit), il ne faut pas utiliser DsfrSearchBar mais un DsfrInput classique.

Petite galère avec DsfrInput, je n'arrive pas à changer son style pour l'instant (mes sélecteurs n'ont aucun effet). Y compris avec :deep(.fr-input) ou même :deep(input) (juste pour l'investigation). Un retour d'expérience ?

@@ -149,11 +149,12 @@ watch(
</div>
</div>
<div class="fr-col-md-12 fr-mb-2w">
<DsfrSearchBar
<SearchComponent
Copy link
Contributor

Choose a reason for hiding this comment

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

Comme commenté, pas certain que la DsfrSearchBar soit adaptée ici.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Désolé @narduin je suis peut être à côté des mes pompes. Mais j'ai remplacé justement DsfrSearchBar par un autre composant. Est-ce que j’interprète mal ton retour ?

@@ -254,11 +254,12 @@ onMounted(() => {
</p>

<div class="fr-col-md-12 fr-mb-2w">
<DsfrSearchBar
<SearchComponent
Copy link
Contributor

Choose a reason for hiding this comment

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

idem: pas certain que la DsfrSearchBar soit adaptée ici.

@narduin
Copy link
Contributor

narduin commented Nov 21, 2024

Petite galère avec DsfrInput, je n'arrive pas à changer son style pour l'instant (mes sélecteurs n'ont aucun effet). Y compris avec :deep(.fr-input) ou même :deep(input) (juste pour l'investigation). Un retour d'expérience ?

Tu peux ajouter une surcharge directement dans le main.css en rajoutant une classe devant pour 'scoper' la surcharge uniquement sur les pages concernées.

Par exemple:

:where(.dataset-list-page, .bouquet-list-page) .fr-search-bar:focus-within {
  outline: 2px solid #0a76f6;
  outline-offset: -2px;
}

:where(.dataset-list-page, .bouquet-list-page) .fr-search-bar input:focus {
  outline: none;
}

@at-github
Copy link
Contributor Author

Petite galère avec DsfrInput, je n'arrive pas à changer son style pour l'instant (mes sélecteurs n'ont aucun effet). Y compris avec :deep(.fr-input) ou même :deep(input) (juste pour l'investigation). Un retour d'expérience ?

Tu peux ajouter une surcharge directement dans le main.css en rajoutant une classe devant pour 'scoper' la surcharge uniquement sur les pages concernées.

Par exemple:

:where(.dataset-list-page, .bouquet-list-page) .fr-search-bar:focus-within {
  outline: 2px solid #0a76f6;
  outline-offset: -2px;
}

:where(.dataset-list-page, .bouquet-list-page) .fr-search-bar input:focus {
  outline: none;
}

Idem, ça n'a pas l'air d'être pris en compte. Je cherche déjà à changer la couleur de bordure qui est noire (quasiment) par défaut. Et mes selecteurs font défaut…

@narduin narduin requested a review from abulte November 28, 2024 16:50
Copy link
Contributor

@abulte abulte 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 encore la régression sur météo : le champ input dans le header ne se vide pas après la recherche.

Also le focus est encore un peu étrange, mais nitpick, ça passe.

Capture d’écran 2024-11-28 à 18 15 44

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.

Mettre à jour le composant barre de recherche sur toutes les pages
4 participants