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

Acantin/recruiter form accessibility fix #371

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

Conversation

AlexandreCantin
Copy link
Contributor

@AlexandreCantin AlexandreCantin commented May 24, 2019

  • Ajout du format attendu sur les champs nécessitant un formatage précis (siret/email/téléphone) avec un lien avec aria-labelledby
  • Précision du statut du formulaire ==> Erreur ou Confirmation dans le title de la page
  • Lien entre les messages et les input concernés avec aria-labelledby
  • Ajout du formatage attendu dans les messages d'erreurs si besoin

@AlexandreCantin
Copy link
Contributor Author

review @kemar @vgrange

@kemar
Copy link
Member

kemar commented May 24, 2019

Juste pour info je suis en train de (beaucoup) modifier des fichiers relatifs à la Demande de modification des métiers que tu as modifié aussi dans cette PR. Je risque d'écraser tes modifs quand je mergerai ma branche sinon ça va être trop violent comme diff...

def compute_input_attributes(field):
attrs = {}

if field.validators and field.validators[0].__class__.__name__ == "DataRequired":
Copy link
Member

Choose a reason for hiding this comment

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

Je pense que tu peux écrire ça mieux avec field.flags.required. À tester !

Copy link
Contributor Author

Choose a reason for hiding this comment

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

En effet, ça marche et c'est plus digeste !

@@ -210,6 +210,29 @@ def inject_user():
def inject_jepostule_enabled():
return {'jepostule_enabled': jepostule_enabled}

def inject_compute_input_attributes():
Copy link
Member

Choose a reason for hiding this comment

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

Ça mérite une docstring qui explique ce que ça fait !

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

# aria-describedby are used to link input fields to their description and/or error messages
aria_describedby = []
if field.description:
aria_describedby.append('{}_description'.format(field.id))
Copy link
Member

@kemar kemar May 24, 2019

Choose a reason for hiding this comment

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

Juste pour la forme, en Python 3 il y a les f-strings qui te permettent d'écrire comme ça :

aria_describedby.append(f"{field.id}_description")

@@ -128,6 +128,8 @@ def change_info():
fix_csrf_session()
form = forms.OfficeIdentificationForm()

form_has_errors = False
Copy link
Member

Choose a reason for hiding this comment

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

Ça me paraît super bizarre, il y a une propriété Form.errors qui existe déjà. Il y a un truc qui cloche si tu te retrouves à coder un mécanisme de détection d'erreurs à côté du natif.

Copy link
Contributor Author

@AlexandreCantin AlexandreCantin May 24, 2019

Choose a reason for hiding this comment

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

Le message d'erreur était un message flash, je l'ai passé en erreur au niveau du champs en question :)

@AlexandreCantin
Copy link
Contributor Author

AlexandreCantin commented May 24, 2019

J'ai fait les changements @kemar, je te laisse finir la PR impactante et je ferai le rebase ensuite.
Bien qu'il y ait beaucoup de changements, ils ne sont pas très lourds et donc facile à récupérer !

Copy link
Member

@kemar kemar left a comment

Choose a reason for hiding this comment

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

Yes nickel les changements ! Comme je te l'ai signalé je suis en train de travailler sur des fichiers que tu as modifié, je tenterai de faire attention pendant le merge mais il y a peut-être certaines de tes modifs qui vont sauter :'(

Sinon LGTM

@dejafait dejafait assigned dejafait and unassigned kemar Sep 2, 2019
@dejafait
Copy link
Contributor

dejafait commented Sep 2, 2019

je vais ressuciter cette PR qui manifestement est passée à la trappe

cc @AlexandreCantin @celine-m-s

@dejafait
Copy link
Contributor

dejafait commented Sep 2, 2019

Wow le merge conflict est gratiné... je retenterais quand j'aurai plus de temps.

@dejafait
Copy link
Contributor

dejafait commented Jan 9, 2020

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.

None yet

3 participants