EnCiv / civil-pursuit

Other
7 stars 8 forks source link

Dynamic Header (H1-6) calculation #80

Open ddfridley opened 9 months ago

ddfridley commented 9 months ago

We are using react components that nest in other react components to implement our discussion pages. However, screen readers want consistency in the use of H2 - H6 headers. (And only a single H1 header on any page).

ddfridley commented 2 months ago

@ldgze How is this going?

ldgze commented 2 months ago

@ldgze How is this going?

Hi David,

I encountered some challenges with the task of identifying new components and determining where to add H and Level:

  1. There is always a global <h1> rendered on the Storybook page (outside our component). As a result, react-accessible-headings cannot adjust accordingly, causing our component’s first header to also render as <h1>. This leads to an accessibility error: (index):691 WCAG accessibility issue detected: skipped heading levels h1,h1. This issue seems to stem from Storybook’s isolated rendering, which does not account for the <h1> in the document. Should I address this issue, or is it acceptable to leave it as is since the error is confined to Storybook and does not affect the component itself?

CleanShot 2024-07-10 at 21 31 55@2x

  1. For components that use the point or point-group components, such as grouping-stepand show-dual-point-list, there is no header for the component itself. As a result, all points have <h1> headers, which causes an accessibility error. How should we handle this situation?
ddfridley commented 2 months ago

For 1: What are the 2 things that render as H1?

For storybook, can we create a decorator that sets Story => that will cause everything in the story to start at level 2?

For 2: The purpose of doing this is so that the points within point group and show-dual-point list will get headers, that will be the right headers. React accessibility header creates a context, so the should be able to get the current level from there. Perhaps the problem is addressed by the above where we need to initially set Level. But we need to figure out some way to make it so they don't show H1.

If you want, you could push some code as draft and I could take a look, or we could do a quick call sometime and look it over.

Thanks,

ddfridley commented 2 months ago

Another thought it that maybe the render of the Point component should start with a

ldgze commented 2 months ago

For 1: What are the 2 things that render as H1?

For storybook, can we create a decorator that sets Story => that will cause everything in the story to start at level 2?

For 2: The purpose of doing this is so that the points within point group and show-dual-point list will get headers, that will be the right headers. React accessibility header creates a context, so the should be able to get the current level from there. Perhaps the problem is addressed by the above where we need to initially set Level. But we need to figure out some way to make it so they don't show H1.

If you want, you could push some code as draft and I could take a look, or we could do a quick call sometime and look it over.

Thanks,

For the page located at http://localhost:6006/?path=/story/point--children-points-six-layers-deep, there is an <h1> element inside the <div class="sb-nopreview sb-wrapper"></div>.

Additionally, there is another<h1> element that represents the first point of the component.

CleanShot 2024-07-12 at 11 34 52@2x

ddfridley commented 2 months ago

I looked at the code. In point-group.jsx, the subject should be rendered as an rather than a div, and then before the places where groupedPoints.map is called there should be a

ldgze commented 2 months ago

I looked at the code. In point-group.jsx, the subject should be rendered as an rather than a div, and then before the places where groupedPoints.map is called there should be a so that all the CreatePoints will be at the next level down.

Hi David, I just pushed the code to update the point-group.jsx.

ldgze commented 2 months ago

I looked at the code. In point-group.jsx, the subject should be rendered as an rather than a div, and then before the places where groupedPoints.map is called there should be a so that all the CreatePoints will be at the next level down.

Hi David, I just pushed the code to update the point-group.jsx.

Problem 1 is also solved by creating a decorator that sets Story to start everything in the story at level 2. The code has been pushed.

ddfridley commented 2 months ago

This is looking good for point and for point-group. I see a similar problem for compare-reasons.jsx - can you look into it. Then with the debugger open, can you go through all the stories and see which ones generate the WCAG accessibility warning and fix them, or bring them up for further discussion? These WCAG warnings will make it easy to find all the areas we need to investigate.

Thanks!

ddfridley commented 2 months ago

I think you might as well put the levelDecorator in .storybook/preview.js so it is applied to all stories. It won't hurt stories that doesn't use Level.

ddfridley commented 1 month ago

@ldgze How's it going? I just merged the point refactor, it touched a lot of files. It may be easier not to merge with master, and when you push the PR I can handle the merge. There are changes to point and point-group among others.

ldgze commented 1 month ago

Hi @ddfridley,

The main tasks for this issue have been completed, and it is ready for review. I have merged the master branch into dynamic-header-h1-6-calculation#80 and resolved all conflicts. However, there are currently two small items that need to be addressed:

  1. I created preview.js and added levelDecorator to the decorator array as you suggested, but it doesn't apply to all the stories by default. I have to manually add it to each story file. Do you have any suggestions on how to debug this?

  2. For components like why-step, show-dual-point-list, and others, when I change the header from <div> to<H>, the font also changes. Should we keep the font consistent? CleanShot 2024-07-28 at 01 18 36@2x

CleanShot 2024-07-28 at 01 17 57@2x

ddfridley commented 1 month ago

Great work! The font and look should be what it was before the change to H.  It needs to look as the designer intended.

I'll take a look at preview.js later.

Thanks

David.

⁣Get BlueMail for Android ​

On Jul 28, 2024, 1:21 AM, at 1:21 AM, Dongze Li @.***> wrote:

Hi @ddfridley,

The main tasks for this issue have been completed, and it is ready for review. I have merged the master branch into dynamic-header-h1-6-calculation#80 and resolved all conflicts. However, there are currently two small items that need to be addressed:

  1. I created preview.js and added levelDecorator to the decorator array as you suggested, but it doesn't apply to all the stories by default. I have to manually add it to each story file. Do you have any suggestions on how to debug this?

  2. For components like why-step, show-dual-point-list, and others, when I change the header from <div> to<H>, the font also changes. Should we keep the font consistent? ![CleanShot 2024-07-28 at 01 18 @.***(https://github.com/user-attachments/assets/e6f7d324-0bb0-481a-9fa9-7c275744ffed)

![CleanShot 2024-07-28 at 01 17 @.***(https://github.com/user-attachments/assets/4f00900e-86e7-44ec-afa6-da25748073b9)

-- Reply to this email directly or view it on GitHub: https://github.com/EnCiv/civil-pursuit/issues/80#issuecomment-2254390767 You are receiving this because you were mentioned.

Message ID: @.***>

ddfridley commented 1 month ago

For preview.js look at .storybook/preview.js (not stories/preview.js).

ddfridley commented 1 month ago

@ldgze I am moving this to the in progress column because I think I forgot to after my last review. If I should review it again let me know. Thanks!

ldgze commented 1 month ago

@ldgze I am moving this to the in progress column because I think I forgot to after my last review. If I should review it again let me know. Thanks!

Thanks David! I will work on it today and tomorrow.

ddfridley commented 3 weeks ago

I keep seeing pushes. That's great. Let me know when this is ready to review again. I'm moving it to the inprogress column.