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

Redesign support table warning #1056

Merged
merged 6 commits into from
May 6, 2024
Merged

Conversation

Paul-Clue
Copy link
Collaborator

@Paul-Clue Paul-Clue commented Apr 23, 2024

see issue #212 (in wai-aria-practices)

This pull request changes the warning at the top of the APG support table so that it matches the style of the warnings on the rest of the page.

@Paul-Clue Paul-Clue requested a review from evmiguel April 23, 2024 02:58
Copy link
Contributor

@evmiguel evmiguel left a comment

Choose a reason for hiding this comment

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

Great work, @Paul-Clue! I have some requests in the comments.

@@ -9,7 +9,7 @@
{{#unless dataEmpty}}
{{#if (isCandidate phase)}}
<details id="embed-report-phase-container">
<summary id="candidate-title"><span>Warning! Unapproved Report</span></summary>
<summary id="candidate-title"><span><span>Warning! Unapproved Report</span></span></summary>
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this could be improved semantically with a heading and a span instead of two spans.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This has been taken care of.

--left-right-padding: 55px;
}

#candidate-title > * {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think instead of applying this to all the children, make this individual margin left and right on the span and icon. I also think that the space between the icon and span can be smaller.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This has been taken care of.

@@ -9,7 +9,7 @@
{{#unless dataEmpty}}
{{#if (isCandidate phase)}}
<details id="embed-report-phase-container">
<summary id="candidate-title"><span>Warning! Unapproved Report</span></summary>
<summary id="candidate-title"><span><h1>Warning! Unapproved Report</h1></span></summary>
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we make this an h4 so it matches the flow of headings in the APG website?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This has been taken care of.

Copy link
Contributor

@evmiguel evmiguel left a comment

Choose a reason for hiding this comment

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

Thanks for addressing my feedback!

@alflennik
Copy link
Contributor

Code-wise it looks great. It's great to be able to use the exact same styling in both places. However there are some differences still. Take a look at the screenshot.

Comparison of existing disclosure and the iframe disclosure

I think we are not currently using the same font, font-size and the border width looks different. Also I'm not 100% convinced by the pill-shaped background on the h4, something feels off about it and I feel like something more similar to the other disclosure might be preferable.

Copy link
Contributor

@alflennik alflennik left a comment

Choose a reason for hiding this comment

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

Code looks great! See comment above for a couple of enhancements I requested.

Copy link
Contributor

@alflennik alflennik left a comment

Choose a reason for hiding this comment

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

Looks fantastic! Thanks for taking the time to get it so polished.

@alflennik alflennik merged commit b8cefcd into development May 6, 2024
2 checks passed
@alflennik alflennik deleted the support-table-warning-redesign branch May 6, 2024 16:56
@howard-e howard-e mentioned this pull request May 28, 2024
howard-e added a commit that referenced this pull request May 28, 2024
Issues addressed:
* #1105, addresses w3c/aria-at#1070
* #1053, addresses w3c/aria-practices#2971
* #1097, addresses #977
* #1095, addresses #991
* #1093, addresses #934
* #1000, addresses #818
* #1089, addresses #992
* #1067, addresses #993
* #1056, addresses w3c/wai-aria-practices#212

---------

Co-authored-by: alflennik <[email protected]>
Co-authored-by: Paul Clue <[email protected]>
Co-authored-by: Mx Corey Frang <[email protected]>
Co-authored-by: Mx. Corey Frang <[email protected]>
Co-authored-by: Erika Miguel <[email protected]>
Co-authored-by: Mike Pennisi <[email protected]>
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