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 delimiter prop to EuiComboBox #3104

Merged
merged 25 commits into from
Mar 25, 2020
Merged

Conversation

anishagg17
Copy link
Contributor

@anishagg17 anishagg17 commented Mar 17, 2020

Summary

Fixes: #1357, functionality can be noticed in the first combo-box example in the demo , (delimenator=",")

Screenshot

when delimenator is input

Hnet-image (3)

Pasting delimenator separated value

Hnet com-image (9)

Copy Functionality

Hnet com-image (11)

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?

@cjcenizal
Copy link
Contributor

CC @sebelga since I believe you had thoughts on similar functionality.

@anishagg17
Copy link
Contributor Author

I know how to implement the copy feature but Can someone suggest me should i use a button with a copy icon or something else?

@anishagg17 anishagg17 changed the title [WIP] added delimenator prop [WIP] added delimiter prop Mar 19, 2020
@anishagg17 anishagg17 changed the title [WIP] added delimiter prop Added delimiter prop to EuiComboBox Mar 19, 2020
@cchaos
Copy link
Contributor

cchaos commented Mar 19, 2020

@anishagg17 The copy/paste ability is just the OS's innate copy/paste ctrl+c. We don't need to provide a UI for it.

@anishagg17
Copy link
Contributor Author

But it was requested

The user can click a "Copy" icon, which will copy a delimited string of the pill values to the clipboard.

@cchaos
Copy link
Contributor

cchaos commented Mar 20, 2020

Ah I see that now in #1357, but I think copying is a rare case and we shouldn't need to provide a UI for it. I've crosse that one out for a simple OS copy ability like requested in #2350

@anishagg17
Copy link
Contributor Author

anishagg17 commented Mar 20, 2020

@cchaos I have already removed that copy button you may check the functionality now

@cchaos
Copy link
Contributor

cchaos commented Mar 20, 2020

Does the current state of this PR support OS copying?

@anishagg17
Copy link
Contributor Author

Not like that in #2350 but we can select text and then copy it

@anishagg17
Copy link
Contributor Author

I think we may do something like if we double click the pill, it turns into an editable pill with that we can change the content of pill too in some future pr

@cchaos
Copy link
Contributor

cchaos commented Mar 20, 2020

Can you then remove the reference to #2350 since it doesn't actually fix the issue?

@anishagg17
Copy link
Contributor Author

removed

@cchaos
Copy link
Contributor

cchaos commented Mar 20, 2020

Jenkins, test this

@kibanamachine
Copy link

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

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.

One weird behavior I noticed is that when I am copy/pasting a coma list, it works fine if I then hit enter. However, if I paste and then type the delimiter , it keeps only the first one:

Screen Capture on 2020-03-20 at 17-45-35

src/components/combo_box/combo_box.tsx 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_delimiter.js Outdated Show resolved Hide resolved
anishagg17 and others added 3 commits March 21, 2020 03:23
Co-Authored-By: Caroline Horn <549577+cchaos@users.noreply.github.com>
Co-Authored-By: Caroline Horn <549577+cchaos@users.noreply.github.com>
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.

This is really nice work and a great improvement! I did a functionality check and even made sure that removing things like onCreateOption still worked while this is enabled. I also checked @cchaos' issues were fixed. I have one small suggestion for when onCreateOption is enabled, delimiter is enabled, and the string has the delimiter present. The text that shows in the dropdown should give better feedback. I'd suggest something like Hit enter to add each item separated by {delimiter}

image

@anishagg17
Copy link
Contributor Author

Screenshot 2020-03-25 at 12 40 41 PM

Changes made

…ons_list.tsx

Co-Authored-By: Caroline Horn <549577+cchaos@users.noreply.github.com>
@cchaos
Copy link
Contributor

cchaos commented Mar 25, 2020

Jenkins test this

@kibanamachine
Copy link

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

@cchaos
Copy link
Contributor

cchaos commented Mar 25, 2020

Might be flakey?

Jenkins test this

@anishagg17
Copy link
Contributor Author

anishagg17 commented Mar 25, 2020

FATAL ERROR: Ineffective mark-compacts near heap limit Allocation failed - JavaScript heap out of memory

I am getting this error while running test command , yesterday also this never gave any output #3104 (comment)

@kibanamachine
Copy link

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

@anishagg17
Copy link
Contributor Author

Changes have been made .

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.

Thanks! LGTM, checking for tests

@cchaos
Copy link
Contributor

cchaos commented Mar 25, 2020

Jenkins test this

@kibanamachine
Copy link

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

@cchaos
Copy link
Contributor

cchaos commented Mar 25, 2020

Jenkins test this

@kibanamachine
Copy link

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

@chandlerprall chandlerprall self-requested a review March 25, 2020 19:39
Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

Changes LGTM; Pulled and tested locally, unable to break anything or get into weird states.

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.

Add "delimited" prop to EuiComboBox and support copy/paste
6 participants