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

Application: adding application template and examples #2267

Merged
merged 1 commit into from
Dec 12, 2023

Conversation

Garneauma
Copy link
Contributor

Adding CDTS application elements inside an "Application" template.

@Garneauma Garneauma temporarily deployed to github-ci October 16, 2023 18:28 — with GitHub Actions Inactive
@duboisp
Copy link
Member

duboisp commented Oct 30, 2023

Pre-approved upon successful review and testing

Copy link
Member

@duboisp duboisp left a comment

Choose a reason for hiding this comment

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

@Garneauma Il faudrait faire une petite documentation rapide afin d'officialiser la version 1 de ce template.


@GormFrank @delisma @bsouster Qu'est-ce que vous pensez de l'utilisation du double h1 pour indiquer le nom de l'application? Ou, est-ce que vous avez une solution alternative qui respecterait le modèle visuelle de cette page tout en étant conforme au WCAG 2.1. Vous pouvez voir le markup proposé via le fichier modifier de ce PR "templates/application/layouts/application.html"

Il faut considérer que ce PR a pour but principal d'éliminer l'utilisation de CSS custom dans CDTS concernant leur application template actuelle et qu'éventuellement nous allons devoir prendre le temps nécessaire afin de revoir entièrement l'application template. L'objectif ici c'est d'avoir rapidement un modèle conforme à l'accessibilité et qui maintient l'expérience visuelle qui a été implémenté dans CDTS et utilisé par les applications web utilisant CDTS.

Voici les examples pratiques

Moi, @duboisp, je crois que l'utilisation du double h1 ici pourrait être satisfaisant pour ce gabarit jusqu'à ce qu'on prenne le temps de revoir ce gabarit dans son entièreté.

templates/application/index.json-ld Outdated Show resolved Hide resolved
templates/application/layouts/application.html Outdated Show resolved Hide resolved
templates/application/_base.scss Outdated Show resolved Hide resolved
templates/application/application-with-banner-fr.html Outdated Show resolved Hide resolved
templates/application/layouts/application.html Outdated Show resolved Hide resolved
@Garneauma
Copy link
Contributor Author

@GormFrank @delisma @bsouster Qu'est-ce que vous pensez de l'utilisation du double h1 pour indiquer le nom de l'application? Ou, est-ce que vous avez une solution alternative qui respecterait le modèle visuelle de cette page tout en étant conforme au WCAG 2.1. Vous pouvez voir le markup proposé via le fichier modifier de ce PR "templates/application/layouts/application.html"

@GormFrank @delisma @bsouster Après discussion avec @duboisp, l'idée d'utiliser le double H1 a été mise de côté. Le nom de l'application utilise maintenant un H2.

@Garneauma Garneauma temporarily deployed to github-ci November 2, 2023 13:44 — with GitHub Actions Inactive
@Garneauma Garneauma requested a review from duboisp November 2, 2023 14:26
templates/application/_base.scss Show resolved Hide resolved
templates/application/application-docs-en.html Outdated Show resolved Hide resolved
@GormFrank
Copy link
Collaborator

@GormFrank @delisma @bsouster Qu'est-ce que vous pensez de l'utilisation du double h1 pour indiquer le nom de l'application? Ou, est-ce que vous avez une solution alternative qui respecterait le modèle visuelle de cette page tout en étant conforme au WCAG 2.1. Vous pouvez voir le markup proposé via le fichier modifier de ce PR "templates/application/layouts/application.html"

@GormFrank @delisma @bsouster Après discussion avec @duboisp, l'idée d'utiliser le double H1 a été mise de côté. Le nom de l'application utilise maintenant un H2.

@Garneauma @duboisp @delisma @bsouster I agree with the H2 approach. I personnally believe that it doesn't cause any semantics problems, as well as UX.

@duboisp
Copy link
Member

duboisp commented Nov 27, 2023

Pre-approve upon review and local testing

Copy link
Member

@duboisp duboisp left a comment

Choose a reason for hiding this comment

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

A few item for discussion.

Also I completed a the local testing and the review. And it look good.

templates/application/layouts/application.html Outdated Show resolved Hide resolved
sites/theme.scss Outdated Show resolved Hide resolved
@duboisp
Copy link
Member

duboisp commented Dec 11, 2023

Pre-approved upon another review and local testing.

Copy link
Member

@duboisp duboisp left a comment

Choose a reason for hiding this comment

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

Reviewed and tested locally,

Everything work as expected. With this change we are going to be able to initiate the removal of custom CSS used by CDTS

@duboisp duboisp merged commit 6ba871d into wet-boew:master Dec 12, 2023
1 check passed
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.

3 participants