Skip to content

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

Merged
cchaos merged 23 commits intoelastic:masterfrom
cchaos:docs/navigation_part2
Aug 25, 2021
Merged

[Docs] Fixing up some more pages under Navigation#5053
cchaos merged 23 commits intoelastic:masterfrom
cchaos:docs/navigation_part2

Conversation

@cchaos
Copy link
Copy Markdown
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
Copy Markdown

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

1 similar comment
@kibanamachine
Copy link
Copy Markdown

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

@kibanamachine
Copy link
Copy Markdown

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

Copy link
Copy Markdown
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.

@@ -1,4 +1,4 @@
import { EuiPagination, EuiText } from '../../../../src/components/';
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

image

/**
* Never accepts children directly, only through the `items` prop
*/
children?: never;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

TIL

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown

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

Copy link
Copy Markdown
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
Copy Markdown

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

@kibanamachine
Copy link
Copy Markdown

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

@kibanamachine
Copy link
Copy Markdown

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
Copy Markdown
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 cchaos enabled auto-merge (squash) August 25, 2021 15:54
@kibanamachine
Copy link
Copy Markdown

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

Development

Successfully merging this pull request may close these issues.

[EuiNotificationBadge] The number inside badge looks not centred, corner radius value is different than in the design component

5 participants