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

Added prepend and append to EuiComboBox #3003

Merged
merged 16 commits into from
Mar 11, 2020

Conversation

ashikmeerankutty
Copy link
Contributor

@ashikmeerankutty ashikmeerankutty commented Mar 7, 2020

Summary

Fixes #2943
Added prepend and append to Combobox if singleSelection is true
Summarize your PR. If it includes design elements include a screenshot or gif.

Checklist

  • Check against all themes for compatibility in both light and dark modes
  • 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

@kibanamachine
Copy link

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

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.

Hey @ashikmeerankutty There is actually a much easier solution for this.

EuiFormControlLayout is the one that provides all the styling for the prepend and append elements. EuiComboBoxInput is the component that renders via EuiFormControlLayout, therefore, you just need to extend the prepend / append prop types from this component and pass them down to EuiComboBoxInput IF singleSelection is true.

@ashikmeerankutty
Copy link
Contributor Author

@cchaos updated Can you please review?

@elizabetdev
Copy link
Contributor

jenkins test this

@kibanamachine
Copy link

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

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.

The code looks right but there's no way to test this. We'll need to add an example to the docs. @miukimiu Can you advise @ashikmeerankutty on what text to add. I'd assume we can just add the display toggles component to the single selection example.

Screen Shot 2020-03-10 at 17 08 33 PM

src/components/combo_box/combo_box.tsx Outdated Show resolved Hide resolved
@elizabetdev
Copy link
Contributor

Hi @ashikmeerankutty,

When I triggered the preview build I noticed there was no way to test it. I had to test it locally and I noticed a few things.

  1. There is a missing euiComboBox__inputWrap--inGroup that should be added to the input wrapper.

111@2x

  1. As @cchaos mentioned there is no way to test the prepend and append. So, the display toggles component should be added to the single selection example.

  2. I also noticed, there are no props notes. Because the prepend and append only work when the singleSelection is true, it's important to document this information.

props@2x

Note

I created a PR ashikmeerankutty#1 that fixes these 3 issues. You can merge it or just follow it as an example.

@kibanamachine
Copy link

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

@ashikmeerankutty
Copy link
Contributor Author

@miukimiu Thanks for the PR and suggestions. @cchaos Changes updated. Can you please review it?

@cchaos
Copy link
Contributor

cchaos commented Mar 11, 2020

Jenkins, test this

@cchaos cchaos changed the title Added prepend and append to Combobox Added prepend and append to EuiComboBox Mar 11, 2020
@kibanamachine
Copy link

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

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
src-docs/src/views/combo_box/combo_box_example.js Outdated Show resolved Hide resolved
src-docs/src/views/combo_box/combo_box_example.js Outdated Show resolved Hide resolved
src-docs/src/views/combo_box/combo_box_example.js Outdated Show resolved Hide resolved
src/components/combo_box/combo_box.tsx Outdated Show resolved Hide resolved
src/components/combo_box/combo_box.tsx Outdated Show resolved Hide resolved
@ashikmeerankutty
Copy link
Contributor Author

@cchaos Fixed 👍

@elizabetdev
Copy link
Contributor

jenkins test this

@kibanamachine
Copy link

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

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.

Looks great, thank you @ashikmeerankutty

@cchaos cchaos merged commit 54b2c64 into elastic:master Mar 11, 2020
@ashikmeerankutty ashikmeerankutty deleted the combobox/append-prepend branch April 15, 2020 09:53
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.

EuiComboBox should allow prepend and append
4 participants