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.

Sign up for GitHub

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

Jump to bottom

calcite-panel: Set the default heading to be a div to accommodate more audiences #5099

Closed
kevindoshier opened this issue Aug 4, 2022 · 7 comments
Closed

calcite-panel: Set the default heading to be a div to accommodate more audiences #5099

kevindoshier opened this issue Aug 4, 2022 · 7 comments
Assignees
@geospatialem @benelan
Labels
4 - verified Issues that have been released and confirmed resolved. a11y Issues related to Accessibility fixes or improvements. breaking change Issues and pull requests with code changes that are not backwards compatible. bug Bug reports for broken functionality. Issues should include a reproduction of the bug. p - medium Issue is non core or affecting less that 60% of people using the library
Milestone
Sprint 2022/11/07...

Comments

@kevindoshier
Copy link
Contributor

Summary

The mapViewer has been undergoing accessibility review, and one of the issues that came up was the headers within calcite-panels which we use all over the app. The comment from those reviewers is

[Issue] There are dialogs present that have headings that do not match their visual level. The first heading starts with <h3> instead of <h2>.

Could we consider setting the default for headingLevel to be 2 to match this review, otherwise we would need to scour everywhere in the app where we use calcite-panels and manually make this change

Actual Behavior

The headingLevel appears to default to 3

Expected Behavior

Default the headingLevel to 2 to satisfy the accessibility review requirements

Reproduction Sample

https://codepen.io/kevindoshier/pen/dymmBwy

Reproduction Steps

  1. Click repro link above

Reproduction Version

beta.86

Working W3C Example/Tutorial

No response

Relevant Info

No response

Regression?

No response
@kevindoshier kevindoshier added 0 - new New issues that need assignment. a11y Issues related to Accessibility fixes or improvements. bug Bug reports for broken functionality. Issues should include a reproduction of the bug. needs triage Planning workflow - pending design/dev review. p - high Issue should be addressed in the current milestone, impacts component or core functionality labels Aug 4, 2022
@macandcheese
Copy link
Contributor

It does seem a bit weird that we have a default set at all. I almost wonder if it should be rendering a plain div / span by default, and only render h1-h6 when a user sets this property.

Ultimately, even if in Map Viewer defaulting to h2 makes sense, it may not be the case for another app, and they would need to adjust that similarly.

@geospatialem geospatialem added p - medium Issue is non core or affecting less that 60% of people using the library and removed p - high Issue should be addressed in the current milestone, impacts component or core functionality labels Oct 12, 2022
@geospatialem geospatialem removed the needs triage Planning workflow - pending design/dev review. label Oct 12, 2022
@geospatialem geospatialem changed the title calcite-panel: consider setting the headingLevel to be 2 for accessibility reasons calcite-panel: Set the default heading to be a span to accommodate more audiences Oct 18, 2022
@jcfranco
Copy link
Member

jcfranco commented Nov 8, 2022

@geospatialem @macandcheese I noticed we didn't update this issue after discussion. 😅 I believe we agreed to not set a default as we can't guarantee it'll be correct. Can you confirm?

@macandcheese
Copy link
Contributor

Correct, I think we said the default internally rendered element would be a div, or span (defer to @geospatialem), and it would only render h1-h6 when requested via prop. No visual changes for any element.

@geospatialem
Copy link
Member

@geospatialem @macandcheese I noticed we didn't update this issue after discussion. 😅 I believe we agreed to not set a default as we can't guarantee it'll be correct. Can you confirm?

Exactly what @macandcheese said (he beat me to it!) 🐇

@jcfranco Good memory, and thanks for catching this one!

Yes, we'd like to have the default be set to a span, and where a heading is defined change it to the appropriate header class (e.g., heading-level="2" would correspond to a h2).

@driskull
Copy link
Member

driskull commented Nov 9, 2022

A div would be better since its a headings are block elements

@driskull driskull self-assigned this Nov 9, 2022
@driskull driskull added 2 - in development Issues that are actively being worked on. and removed 0 - new New issues that need assignment. labels Nov 9, 2022
driskull added a commit that referenced this issue Nov 9, 2022
@driskull
fix: Set default heading to be a div to accommodate more audiences #5099
1fee791 


Applies to block, date-picker, list-item-group, panel, pick-list-group, popover, tip, and tip-manager
@driskull driskull mentioned this issue Nov 9, 2022
Merged
@driskull driskull added the breaking change Issues and pull requests with code changes that are not backwards compatible. label Nov 10, 2022
@geospatialem geospatialem mentioned this issue Nov 14, 2022
Closed
driskull added a commit that referenced this issue Nov 16, 2022
@driskull
fix(block, date-picker, list-item-group, panel, pick-list-group, popo…
38ca639 
…ver, tip, tip-manager)!: Set default internal heading to a div. (#5728)

* fix: Set default heading to be a div to accommodate more audiences #5099

Applies to block, date-picker, list-item-group, panel, pick-list-group, popover, tip, and tip-manager

* revert change to file
@driskull driskull added 3 - installed Issues that have been merged to master branch and are ready for final confirmation. and removed 2 - in development Issues that are actively being worked on. labels Nov 16, 2022
@github-actions
Copy link
Contributor

Installed and assigned for verification.

@geospatialem geospatialem changed the title calcite-panel: Set the default heading to be a span to accommodate more audiences calcite-panel: Set the default heading to be a div to accommodate more audiences Nov 17, 2022
@geospatialem geospatialem added 4 - verified Issues that have been released and confirmed resolved. and removed 3 - installed Issues that have been merged to master branch and are ready for final confirmation. labels Nov 17, 2022
@geospatialem
Copy link
Member

Verified on next.634 for:
block, date-picker, list-item-group, panel, pick-list-group, popover, tip, and tip-manager. (Also flow-item and input-date-picker. 👍🏻

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@geospatialem geospatialem

@benelan benelan

Labels
4 - verified Issues that have been released and confirmed resolved. a11y Issues related to Accessibility fixes or improvements. breaking change Issues and pull requests with code changes that are not backwards compatible. bug Bug reports for broken functionality. Issues should include a reproduction of the bug. p - medium Issue is non core or affecting less that 60% of people using the library
Projects
None yet
Milestone
Sprint 2022/11/07 - 2022/11/18
Development

No branches or pull requests
6 participants
@jcfranco @driskull @macandcheese @geospatialem @benelan @kevindoshier