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

EuiInputPopover docs #2269

Merged
merged 11 commits into from
Sep 5, 2019
Merged

Conversation

thompsongl
Copy link
Contributor

@thompsongl thompsongl commented Aug 28, 2019

Summary

The beginnings of EuiInputPopover were merged so that EuiSuggest and a compressed variant of EuiRange could use them in their respective feature branches.

For the most part, it's a preconfigured EuiPopover with some resizing hooks. Focus/a11y is largely dependent on the nature of the input provided, so no real assumptions have been made. With that in mind, I'm considering adding the beta tag to this. It's in master, but unused and undocumented. Adding documentation means it needs to provide some sense of its stability, and it's hard to tell with so few known and upcoming use cases; I could see the API changing a bit.

Checklist

- [ ] Checked in dark mode
- [ ] Checked in mobile
- [ ] Checked in IE11 and Firefox

  • Props have proper autodocs
  • Added documentation 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

@thompsongl thompsongl requested a review from cchaos August 28, 2019 21:34
Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

I don't think we need a Beta label. It's not that experimental and I think your paragraph about focus keyboard nav is explanation enough.

Just had a few notes.

src-docs/src/views/popover/popover_example.js Outdated Show resolved Hide resolved
src-docs/src/views/popover/popover_example.js Outdated Show resolved Hide resolved
src-docs/src/views/popover/popover_example.js Outdated Show resolved Hide resolved
@thompsongl thompsongl marked this pull request as ready for review September 4, 2019 19:51
Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

Just a couple spelling mistakes and you still need a changelog entry

src-docs/src/views/popover/popover_example.js Outdated Show resolved Hide resolved
src-docs/src/views/popover/popover_example.js Outdated Show resolved Hide resolved
Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

LGTM, just a nit on the CL

CHANGELOG.md Outdated Show resolved Hide resolved
Co-Authored-By: Caroline Horn <[email protected]>
@thompsongl thompsongl merged commit 4ac7d29 into elastic:master Sep 5, 2019
thompsongl added a commit to thompsongl/eui that referenced this pull request Sep 10, 2019
* first pass

* snippet

* comments

* more spacing before window edge

* Update src-docs/src/views/popover/popover_example.js

Co-Authored-By: Caroline Horn <[email protected]>

* Update src-docs/src/views/popover/popover_example.js

Co-Authored-By: Caroline Horn <[email protected]>

* CL

* Update CHANGELOG.md

Co-Authored-By: Caroline Horn <[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.

2 participants