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

feat: Aria compliant breadcrumb component #157

Merged
merged 14 commits into from
Sep 11, 2022

Conversation

jelmerveen
Copy link
Contributor

Implemented now with feedback and according to the aria specification:

https://www.w3.org/TR/wai-aria-practices/examples/breadcrumb/index.html

@jelmerveen jelmerveen mentioned this pull request Feb 8, 2022
@jelmerveen jelmerveen changed the title Aria compliant breadcrumb component feat: Aria compliant breadcrumb component Feb 8, 2022
@lucaslarroche
Copy link
Member

Closes #144

@lucaslarroche
Copy link
Member

@jelmerveen,
Thanks for this new PR, and sorry for the delay. I didn't have time to review the PRs.

aria-label="breadcrumb"

Nice find. You're right aria-label="Breadcrumb" seems to be the proper practice.
Weirdly the documentation uses Pascalcase ("Breadcrumb") when we would expect a lowercase first letter "breadcrumb".

The example in the accessible breadcrumb pattern referenced on the w3c page however uses lower case 🤔.
Bootstrap also recommends the lowercase version.

I'd instead use the lowercase version if you're ok with it.

!important

We can avoid !important as it is unnecessary (tested).
And when necessary, Pico uses the flag $enable-important to enable/disable !important in the output - Example.

a::after {
- color: var(--muted-color) !important;
+ color: var(--muted-color);
}
a[aria-current="page"] {
-  color: inherit !important;
+  color: inherit;
}

RTL (right-to-left)

Using padding-inline-start instead of padding-left would allow this feature to support RTL automatically.

ul li {
- padding-left: 0;
+ padding-inline-start: 0;
  
  &:not(:last-child) {
    a::after {
-     padding-left: 0.5rem;
+     padding-inline-start: 0.5rem;
    }
  }
}

Let me know if you want to make the changes.

@lucaslarroche lucaslarroche added the work in progress This PR cannot be merged as is label May 21, 2022
@jelmerveen
Copy link
Contributor Author

@lucaslarroche thanks for the great feedback!

I have made the changes, and updated with the dev branch, since it was already a long time.
Can you please verify that the merge with dev is ok? I had to rework some conflicts.

Else please feel free to update it accordingly!

@lucaslarroche lucaslarroche self-requested a review June 13, 2022 03:03
@lucaslarroche
Copy link
Member

Hi @jelmerveen,
Thank you for the changes, and thank you for your patience.
I don't have the time I'd like to update Pico.

I rebased the branch, rebuild the CSS, and changed the style of code snippets.
LGTM.

I will merge into dev, and then in the next release quickly.

@lucaslarroche lucaslarroche added enhancement New feature or request and removed work in progress This PR cannot be merged as is labels Sep 11, 2022
@lucaslarroche lucaslarroche merged commit c378a50 into picocss:dev Sep 11, 2022
@lucaslarroche lucaslarroche mentioned this pull request Sep 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants