PublicisSapient / enable-a11y

MIT License
11 stars 5 forks source link

Feature/html5 autocomplete #146

Open josiahwilliams opened 1 week ago

josiahwilliams commented 1 week ago
Screenshot 2024-06-28 at 11 20 54 AM

http://localhost:8888/autocomplete.php

alisonhall commented 1 week ago

We definitely need to discuss including tailwind before merging this PR. It makes the code walkthrough experience a lot more bloated and hard to read, and it also changes other styles on the page (like the h1).

Is there any way we can not include tailwind at this time?

alisonhall commented 1 week ago

The code walkthrough steps currently talk more about how to create a form and all of the attributes, rather than how to include the autocomplete functionality and how it's accessible. The code highlighting of the steps also highlights a lot of extra lines that shouldn't be highlighted. We should probably rethink what steps are actually needed for autocomplete.

josiahwilliams commented 1 week ago

We definitely need to discuss including tailwind before merging this PR. It makes the code walkthrough experience a lot more bloated and hard to read, and it also changes other styles on the page (like the h1).

Is there any way we can not include tailwind at this time?

Yes agreed. I did this to quickly get a form up there without having to worry about styling. But with the new React codebase being worked on, Tailwinds would be a nice addition to the POSTCSS workflow, esp. for a library with a focus on accessibility. We can chat with Zoltan about this when he is back next week.

josiahwilliams commented 1 week ago

Now i am getting this error:

`Testing http://192.168.1.20:8888/autocomplete.php ... please wait, this may take a minute.

Violation of "heading-order" with 1 occurrences! Ensures the order of headings is semantically correct. Correct invalid elements at:

3 Accessibility issues detected.

{ "Test Runner": { "name": "axe" }, "Test Engine": { "name": "axe-core", "version": "4.8.4" }, "Test Environment": { "userAgent": "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) HeadlessChrome/126.0.6478.114 Safari/537.36", "windowWidth": 800, "windowHeight": 600, "orientationAngle": 0, "orientationType": "landscape-primary" } } ` But it doesnt seem to be related to my code.

cc: @alisonhall

alisonhall commented 1 week ago

Now i am getting this error:

`Testing http://192.168.1.20:8888/autocomplete.php ... please wait, this may take a minute.

Violation of "heading-order" with 1 occurrences! Ensures the order of headings is semantically correct. Correct invalid elements at: - #developer-walkthrough-1 For details, see: https://dequeuniversity.com/rules/axe/4.8/heading-order

Violation of "link-in-text-block" with 2 occurrences! Ensure links are distinguished from surrounding text in a way that does not rely on color. Correct invalid elements at: - a[href$="useragentman.com"] - footer > a:nth-child(2) For details, see: https://dequeuniversity.com/rules/axe/4.8/link-in-text-block

3 Accessibility issues detected.

{ "Test Runner": { "name": "axe" }, "Test Engine": { "name": "axe-core", "version": "4.8.4" }, "Test Environment": { "userAgent": "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) HeadlessChrome/126.0.6478.114 Safari/537.36", "windowWidth": 800, "windowHeight": 600, "orientationAngle": 0, "orientationType": "landscape-primary" } } ` But it doesnt seem to be related to my code.

cc: @alisonhall

Hi @josiahwilliams ,

For the "heading order" issue, it is likely related to your code having headings such as <h1 class="tw-text-3xl tw-font-bold">Checkout</h1> within your autocomplete page. The H1 tag is already autogenerated for the page (and you can only have 1 H1 on a page), and all headings must increase by one step at a time. You might want to install and run the Wave accessibility checker (a browser extension) on your page to help see where there are problems like this.

For the "link-in-text-block" issue, I believe it's due to the page-wide color changes that happened due to you including Tailwind. It has changed the styling of existing components on the page (like the header and footer), and these new styles don't seem to be accessible.

zoltan-dulac commented 1 week ago
Screenshot 2024-06-21 at 7 27 27 AM

http://localhost:8888/autocomplete.php

Screenshot 2024-06-21 at 7 27 27 AM

http://localhost:8888/autocomplete.php

I really like the layout here. Thank you.

josiahwilliams commented 1 week ago

Now i am getting this error: Testing http://192.168.1.20:8888/autocomplete.php ... please wait, this may take a minute. Violation of "heading-order" with 1 occurrences! Ensures the order of headings is semantically correct. Correct invalid elements at: - #developer-walkthrough-1 For details, see: https://dequeuniversity.com/rules/axe/4.8/heading-order Violation of "link-in-text-block" with 2 occurrences! Ensure links are distinguished from surrounding text in a way that does not rely on color. Correct invalid elements at: - a[href$="useragentman.com"] - footer > a:nth-child(2) For details, see: https://dequeuniversity.com/rules/axe/4.8/link-in-text-block 3 Accessibility issues detected. { "Test Runner": { "name": "axe" }, "Test Engine": { "name": "axe-core", "version": "4.8.4" }, "Test Environment": { "userAgent": "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) HeadlessChrome/126.0.6478.114 Safari/537.36", "windowWidth": 800, "windowHeight": 600, "orientationAngle": 0, "orientationType": "landscape-primary" } } But it doesnt seem to be related to my code. cc: @alisonhall

Hi @josiahwilliams ,

For the "heading order" issue, it is likely related to your code having headings such as <h1 class="tw-text-3xl tw-font-bold">Checkout</h1> within your autocomplete page. The H1 tag is already autogenerated for the page (and you can only have 1 H1 on a page), and all headings must increase by one step at a time. You might want to install and run the Wave accessibility checker (a browser extension) on your page to help see where there are problems like this.

For the "link-in-text-block" issue, I believe it's due to the page-wide color changes that happened due to you including Tailwind. It has changed the styling of existing components on the page (like the header and footer), and these new styles don't seem to be accessible.

I was able to fix the Heading issue. The link issue is still happening, even though it is passing in WAVE:

Screenshot 2024-06-24 at 1 58 34 PM

Also, I am prefixxing tailwinds with 'tw-' so that the styles don't affect the other areas of the site.

Digging into this more.

josiahwilliams commented 1 week ago

Now i am getting this error: Testing http://192.168.1.20:8888/autocomplete.php ... please wait, this may take a minute. Violation of "heading-order" with 1 occurrences! Ensures the order of headings is semantically correct. Correct invalid elements at: - #developer-walkthrough-1 For details, see: https://dequeuniversity.com/rules/axe/4.8/heading-order Violation of "link-in-text-block" with 2 occurrences! Ensure links are distinguished from surrounding text in a way that does not rely on color. Correct invalid elements at: - a[href$="useragentman.com"] - footer > a:nth-child(2) For details, see: https://dequeuniversity.com/rules/axe/4.8/link-in-text-block 3 Accessibility issues detected. { "Test Runner": { "name": "axe" }, "Test Engine": { "name": "axe-core", "version": "4.8.4" }, "Test Environment": { "userAgent": "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) HeadlessChrome/126.0.6478.114 Safari/537.36", "windowWidth": 800, "windowHeight": 600, "orientationAngle": 0, "orientationType": "landscape-primary" } } But it doesnt seem to be related to my code. cc: @alisonhall

Hi @josiahwilliams , For the "heading order" issue, it is likely related to your code having headings such as <h1 class="tw-text-3xl tw-font-bold">Checkout</h1> within your autocomplete page. The H1 tag is already autogenerated for the page (and you can only have 1 H1 on a page), and all headings must increase by one step at a time. You might want to install and run the Wave accessibility checker (a browser extension) on your page to help see where there are problems like this. For the "link-in-text-block" issue, I believe it's due to the page-wide color changes that happened due to you including Tailwind. It has changed the styling of existing components on the page (like the header and footer), and these new styles don't seem to be accessible.

I was able to fix the Heading issue. The link issue is still happening, even though it is passing in WAVE:

Screenshot 2024-06-24 at 1 58 34 PM

Also, I am prefixxing tailwinds with 'tw-' so that the styles don't affect the other areas of the site.

Digging into this more.

I was able to fix this by preventing Tailwind reset css from running. This fixes the link contrast issue and also the ENABLE header logo shift that was happening.

zoltan-dulac commented 1 week ago

I did this to quickly get a form up there without having to worry about styling. But with the new React codebase being worked on, Tailwinds would be a nice addition to the POSTCSS workflow, esp. for a library with a focus on accessibility. We can chat with Zoltan about this when he is back next week.

I would like to hold off on using Tailwind for this project. I would rather focus on using BEM style CSS to style our components here, as we have been doing.

Tailwind is not needed/wanted in a lot of projects, and for that reason I don't want it as a dependency for this project. We want the barrier to entry for using Enable components to be low, which means not a lot of supporting libraries whenever possible.

For context on my decision: it reminds me of the bad old days in the 1990s before CSS where we had to use a bunch of HTML attributes to style stuff (like . In my opinion, Tailwind is an anti-pattern.

I am more than happy to have a discussion to discuss this further if you like. But be warned: I have strong opinions on this.

josiahwilliams commented 5 days ago

We definitely need to discuss including tailwind before merging this PR. It makes the code walkthrough experience a lot more bloated and hard to read, and it also changes other styles on the page (like the h1).

Is there any way we can not include tailwind at this time?

Tailwinds has been removed from this PR.

josiahwilliams commented 5 days ago

We definitely need to discuss including tailwind before merging this PR. It makes the code walkthrough experience a lot more bloated and hard to read, and it also changes other styles on the page (like the h1).

Is there any way we can not include tailwind at this time?

Tailwind has been removed and replaced with BEM and LESS.