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

[Docs] Fixing up some more pages under Navigation #5053

Merged
merged 23 commits into from
Aug 25, 2021

Conversation

cchaos
Copy link
Contributor

@cchaos cchaos commented Aug 19, 2021

A. The main docs pages that were touched were:

  1. Breadcrumbs
    a. I also removed all the aria-labels from the snippets because EuiBreadcrumbs has a default value so it's not absolutely necessary to provide a custom one. I confirmed with @myasonik (😭) that we only add custom ones in our examples because you can't have multiple on the same page with the same label. But I did add a callout about it.
  2. Facets
  3. Pagination
  4. Tree view
  5. Overflow scroll utility classes (added more a11y guidance)

B. Other related changes

1. EuiNotificationBadge

Closes #4992 by reducing the border-radius in Amsterdam theme
image

2. EuiPagination

Fixed the default active page to the actual first page. The activePage is actually 0 based index, but the default was set to 1 making it default to selecting page 2. 🤣

3. EuiTreeView

This is a component that controls the render of it's items through an object passed as a prop. While children was actually allowed via TS, it would error if you tried to pass any. So I set it to never allow children with children?: never;

4. EuiFacet

Updated the focus states in Amsterdam to match our established patter.
image

5. Changed doc's snippet code language from html to jsx

Mainly because nested objects made it very confused.
image

6. Reduced Playground code from 4 space indents to 2 spaces

Checklist

  • Check against all themes for compatibility in both light and dark modes
  • Checked in mobile
  • Checked in Chrome, Safari, Edge, and Firefox
  • Props have proper autodocs and playground toggles
  • Added documentation
  • Checked Code Sandbox works for the any docs examples
  • Added or updated jest tests
  • Checked for breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5053/

1 similar comment
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5053/

CHANGELOG.md Show resolved Hide resolved
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5053/

Copy link
Contributor

@snide snide left a comment

Choose a reason for hiding this comment

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

Didn't know if you were ready for review, but I had some time. Ran through this in code / docs. Looks pretty good. Think the treeview playground is the only place I noticed any issues.

Looks like you also made a bunch of changes to the accessibility of the docs (mostly around some of our utility pages). Might want to comment that up in your PR description.

Really nice work.

src-docs/src/views/breadcrumbs/playground.js Show resolved Hide resolved
@@ -1,4 +1,4 @@
import { EuiPagination, EuiText } from '../../../../src/components/';
Copy link
Contributor

Choose a reason for hiding this comment

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

image

src-docs/src/views/tree_view/playground.js Show resolved Hide resolved
/**
* Never accepts children directly, only through the `items` prop
*/
children?: never;
Copy link
Contributor

Choose a reason for hiding this comment

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

TIL

Copy link
Contributor

Choose a reason for hiding this comment

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

Tried out some Omit options, but this appears to be the best way to remove support.

@cchaos
Copy link
Contributor Author

cchaos commented Aug 20, 2021

Looks like you also made a bunch of changes to the accessibility of the docs (mostly around some of our utility pages).

I'm starting to think we might need an actual EuiOverflow component... 😬

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5053/

Copy link
Contributor

@snide snide left a comment

Choose a reason for hiding this comment

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

LGTM

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5053/

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5053/

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5053/

@thompsongl thompsongl self-requested a review August 25, 2021 14:53
/**
* Never accepts children directly, only through the `items` prop
*/
children?: never;
Copy link
Contributor

Choose a reason for hiding this comment

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

Tried out some Omit options, but this appears to be the best way to remove support.

CHANGELOG.md Show resolved Hide resolved
@cchaos cchaos enabled auto-merge (squash) August 25, 2021 15:54
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5053/

@cchaos cchaos merged commit db1f6f4 into elastic:master Aug 25, 2021
@cchaos cchaos deleted the docs/navigation_part2 branch August 25, 2021 17:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants