-
Notifications
You must be signed in to change notification settings - Fork 844
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] Selection controls #5607
Conversation
Preview documentation changes for this PR: https://eui.elastic.co/pr_5607/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_5607/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_5607/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_5607/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I left a few small comments. Thanks for putting this together!
description={ | ||
<p> | ||
The label should be static, action-oriented, and describe the | ||
feature or present a question. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have an example where the label is (or should be) a question?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point. I couldn't find in our products any example where we presented a question so for now I deleted this text.
@cchaos this text was already in our docs, do you have any example of when we should present a question?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤷♀️ 🤣
Co-authored-by: florent-leborgne <florent.leborgne@elastic.co>
Co-authored-by: florent-leborgne <florent.leborgne@elastic.co>
…le.js Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>
…le.js Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>
…le.js Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>
…le.js Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>
…le.js Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>
…le.js Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>
…le.js Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>
Preview documentation changes for this PR: https://eui.elastic.co/pr_5607/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_5607/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_5607/ |
Thanks, @cchaos for all the help and suggestions. I address all of them. But... I had some difficulties addressing the playground console log warnings. I was able to address the EuiCheckbox and EuiRadio It gets a warning:
|
|
||
propsToUse.onChange = { | ||
...propsToUse.onChange, | ||
value: 'onChange', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually don't think you need this configuration at all because the the value
you're passing is actually a string not function. I think the dummy function is coming from line 85:
customProps: {
onChange: dummyFunction,
},
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Just found some last text fixes.
text="In a list of already created items, use the past tense." | ||
minHeight={234} | ||
> | ||
<EuiBasicTable columns={columns} items={items} tableLayout="auto" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😍
src-docs/src/views/selection_controls/selection_controls_example.js
Outdated
Show resolved
Hide resolved
Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>
…le.js Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>
Preview documentation changes for this PR: https://eui.elastic.co/pr_5607/ |
Summary
This PR adds a new section called selection controls. It's basically a follow-up on #5558 where we were adding switch guidelines. But we started having some issues, just the EuiSwitch had text in the examples section. The same was happening in the guidelines tab.
So I decided to go ahead and add texts for all the components in the examples tab. Also, more generic guidelines to cover other components.
Decisions to make
Right now in the selection controls section lives:
Should the EuiSelect move out from form controls section and move to this new section?
If we decided to move the EuiSelect to this section let's do it in another PR. Just to not overcomplicate.
Checklist
[ ] Checked in mobile[ ] Props have proper autodocs and playground toggles[ ] Checked Code Sandbox works for any docs examples[ ] Added or updated jest and cypress 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