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

Allow pasting multiple lines into the search field for tokenization #5806

Merged
merged 26 commits into from Jan 14, 2021

Conversation

acolchagoff
Copy link
Contributor

@acolchagoff acolchagoff commented Mar 20, 2020

This pull request includes a

  • Bug fix
  • New feature
  • Translation

The following changes were made

-Changes input to select

If this is related to an existing ticket, include a link to it as well.

@acolchagoff
Copy link
Contributor Author

stalebot closed my previous PR

@acolchagoff
Copy link
Contributor Author

Not Stale.

@acolchagoff
Copy link
Contributor Author

Hey @kevin-brown I've noticed that you keep touching the piece of code that this does, and while I'm happy to keep this updated on my end, do you think that it might be a good time to merge it in? Are there any apprehensions you have to merging it? is there anything you want me to change?

@acolchagoff
Copy link
Contributor Author

acolchagoff commented Apr 30, 2020

Also I see with your latest changes I'm no longer allowed to reference 'this' can you advise on how exactly I should access the options in that context?
Should I just change the input to a textarea? the setting was @alexweissman's idea and I did it so that I could get this merged, but a textarea is really better all around because it doesn't drop tab and newline characters.

@acolchagoff
Copy link
Contributor Author

reopened form #4795

@kevin-brown
Copy link
Member

Hi again, it's been a while. I haven't merged this in yet because I have some hesitations around side-effects. And I honestly haven't been in the mood to try to walk through this manually and get it running locally.

  • This needs to be styled to match the existing search box, and that includes not allowing it to actually go across multiple lines
  • Do we know what the accessibility side effects of this are? Pretty much all of the examples around accessibility within a comboxbox involves an <input /> and I'd rather not break all of the work we've done in this area to align us with the standards.
  • We shouldn't allow for both <input /> and <textarea>, that just sounds like a mess. We need to be aware of the fact that this will be a breaking change and therefore right now, as we prepare for 4.1.0, is the best time to consider making this change.
  • Does this have any impact on the keyboard shortcuts that we support?

@acolchagoff
Copy link
Contributor Author

@kevin-brown Thanks for the response, I'll change this patch back into a change rather than a configurable and work on some modeling to show if it effects css.

@kevin-brown kevin-brown changed the title Fixes #3712 Allow pasting multiple lines into the search field for tokenization May 5, 2020
@kevin-brown
Copy link
Member

@denodster I give you credit for following this patch through for 3 years (and counting) and I apologize for that. I can help you out a bit with the styling if we can get the functional parts of it nailed down (keyboard accessibility + shortcuts).

@stale
Copy link

stale bot commented Jul 6, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the status: stale label Jul 6, 2020
@acolchagoff
Copy link
Contributor Author

Not Stale.

@stale stale bot removed the status: stale label Jul 7, 2020
@stale
Copy link

stale bot commented Sep 5, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the status: stale label Sep 5, 2020
@kevin-brown
Copy link
Member

Still not stale.

@stale stale bot removed the status: stale label Sep 5, 2020
@Apezdr
Copy link

Apezdr commented Sep 17, 2020

Any chance of this getting attention?

@acolchagoff
Copy link
Contributor Author

acolchagoff commented Sep 18, 2020

@Apezdr I need to put together some testing of some kind, though I've got other stuff going on right now, if you are inclined to put together a side by side where you compare this version with the main trunk that would be helpful. we need to show that this changes doesn't break screen readers and css.

@stale
Copy link

stale bot commented Nov 21, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the status: stale label Nov 21, 2020
@acolchagoff
Copy link
Contributor Author

Not Stale

@stale stale bot removed the status: stale label Nov 25, 2020
@kevin-brown
Copy link
Member

Still watching this, thanks for keeping it not stale. 👍

This still has an issue with overflows and how the browser will
completely shift focus rather than slowly move it character by
character, but this is something which we can work out at a later
time and shouldn't break things for now.
These tests currently fail in the automated test runner because they
are checking the `autocomplete` property on the `<textarea>` DOM
element. This attribute is widely supported across our supported
browsers, but the property on the JS DOM element has only recently
started to be supported. As a result, the virtual browser used by the
test runner does not support this property and cannot read it for the
tests.
@kevin-brown
Copy link
Member

I pushed up a handful of commits but there is still a failing test around sizing where the text box is now a few pixels too wide all of a sudden. I don't currently have the time to look into this (it's getting late) but I'm planning on tackling this next time, since I want to get this into the next beta of Select2 4.1.0.

@acolchagoff
Copy link
Contributor Author

@kevin-brown thanks for doing that, I've been swamped lately with other things at my company but I still intend to return to this, I'll try to take a look as soon as my schedule gives me the opportunity.

The width was incorrect for the placeholders because it was using the
default browser style instead of the ones from Select2. As a result, it
was coming back with a width of 104 because it was at 100% of 100px
with the default 2px padding all around that is browser by the browser
user style sheets. Now that it is being placed inside of the proper
container, we can see that the expected styles are being applied.
This new class allows us to not have to apply the padding for the
clear icon when the clear icon is not being displayed.
@kevin-brown
Copy link
Member

No worries, I was busy for the last few months so I totally understand.

I think I figured out what was causing the failing test and corrected that issue. I'm going to let this stay open for a little longer (not sure if minutes or days) so eyes can check it out and then I'll merge it in for the next release.

@kevin-brown kevin-brown merged commit 2734538 into select2:develop Jan 14, 2021
anttikuuskoski pushed a commit to anttikuuskoski/select2 that referenced this pull request Mar 29, 2022
…elect2#5806)

* Fixes select2#3712 Allowing Pasting tags from spreadsheet

I was trying to make an easy way for clients to copy and paste tags from a spreadsheet, but I was unable to do this because select2's use of a 'search' input would remove all newline characters from the pasted input. replacing the search text input with a textarea removes this limitation.

* Fixes bug and broken tests

* Missed one spot

* Fixes tests posibly?

* reformat long lines

* Reference options instead of searchElement directly

* make input come first in selector

* Make textarea the default rather than a configurable

* fix weird whitespace

* missing a quote

* Fix styling to be closer to the default for search boxes

This still has an issue with overflows and how the browser will
completely shift focus rather than slowly move it character by
character, but this is something which we can work out at a later
time and shouldn't break things for now.

* Fix tests expecting to find an input

* Skip autocomplete property tests

These tests currently fail in the automated test runner because they
are checking the `autocomplete` property on the `<textarea>` DOM
element. This attribute is widely supported across our supported
browsers, but the property on the JS DOM element has only recently
started to be supported. As a result, the virtual browser used by the
test runner does not support this property and cannot read it for the
tests.

* Fix placeholder test returning incorrect width

The width was incorrect for the placeholders because it was using the
default browser style instead of the ones from Select2. As a result, it
was coming back with a width of 104 because it was at 100% of 100px
with the default 2px padding all around that is browser by the browser
user style sheets. Now that it is being placed inside of the proper
container, we can see that the expected styles are being applied.

* Introduce new class to better handle the clear button

This new class allows us to not have to apply the padding for the
clear icon when the clear icon is not being displayed.

* Update test with proper width of the search box

Co-authored-by: Kevin Brown <kevin-brown@users.noreply.github.com>
Co-authored-by: Andrew Colchagoff <andrew@tryskillo.com>
Co-authored-by: Kevin Brown <kevin@kevin-brown.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants