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

Add scrollAfterSelect as a configurable option for multiselect dropdowns to allow toggling of highlightFirstItem() behaviour #5150

Merged
merged 3 commits into from Sep 5, 2018

Conversation

boweihan
Copy link
Contributor

@boweihan boweihan commented Nov 29, 2017

Resolves issue for multiselects using closeOnSelect: false that caused the list of results to scroll to the first selection after each select/unselect. This behaviour was intentional to deal with infinite scroll UI issues but it created an issue with multiselect dropdown boxes of fixed length. This pull request adds a configurable option to toggle between these two desirable behaviours.

This pull request includes a

  • Bug fix
  • New feature
  • Translation

The following changes were made

  • Added configurable option "scrollAfterSelect"
  • If set to TRUE multiselect dropdowns will scroll to the first item on the list after "select" or "unselect" events (i.e. for infinite scroll case)
  • If set to FALSE highlight will remain on selected item.

I ran into this issue when implementing select2 multiselect dropdowns with a fixed item length. It was confusing to have the focus move to the first item after each select/unselect event.

If this is related to an existing ticket, include a link to it as well.
#4869 - is tagged needs more work. I noticed it hasn't been touched for a while so I'm following up on this.
#4417 - discussion for where this problem originated. (this particular commit - 9f58128)

@alexweissman
Copy link
Contributor

Nice! Can you add some tests for this?

@boweihan boweihan changed the title Add scrollOnSelect as a configurable option for multiselect dropdowns to allow toggling of highlightFirstItem() behaviour Add scrollAfterSelect as a configurable option for multiselect dropdowns to allow toggling of highlightFirstItem() behaviour Dec 6, 2017
@boweihan
Copy link
Contributor Author

boweihan commented Dec 6, 2017

@alexweissman I added some tests to focusing-tests.js to verify the configurable option behaviour. I also changed the default for scrollAfterSelect to be "true" to avoid modifying existing implementations (this was debatable to me since it means that anyone using a non-paginating dropdown will need to explicitly set { scrollAfterSelect: false} but I feel it's justifiable to avoid introducing breaking changes for pagination users).

@alexweissman
Copy link
Contributor

I have to say though, I am confused by the explanation given here: #4417 (comment)

@jgbishop and @AlexanderLeonov, can you explain exactly why the highlightFirstItem method ever needs to be called, even in the infinite scroll (pagination) case?

@calumah
Copy link

calumah commented Feb 20, 2018

Any update on this PR ?

I'm using large multi-select with only few value to select and this bug/feature is very annoying for users :/

I'm trying to do a monkey patching for disabling highlightFirstItem() function but I can't find the correct syntax / path ...

@jsJunky
Copy link

jsJunky commented Mar 15, 2018

Sorry if this is spamming but I was wondering what the progress of this PR for acceptance is.

@pedrofurtado
Copy link
Contributor

@kevin-brown @alexweissman Is this PR ready for merge? If so, merge it, please!

Copy link

@marciojg marciojg left a comment

Choose a reason for hiding this comment

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

@boweihan in my tests I saw that it was necessary to add 'scrollAfterSelect' validation in calls to 'highlightFirstItem()' also in the file 'dist\js\select2.js'

@jsJunky
Copy link

jsJunky commented Mar 21, 2018

@marciojesus Would that not be done during a build phase?

@marciojg
Copy link

@jsJunky I don't understand your question very well. But what I meant is that only worked for me when I added of 'scrollAfterSelect' validation in call of 'highlightFirstItem()' in the the file 'dist\js\select2.js' unlike of 'src/js/select2/results.js' :)

@jsJunky
Copy link

jsJunky commented Mar 22, 2018

@marciojesus Usually most projects do not require you to manually update the final built files. Those will be handled by the build system that will grab all necessary files and build them into a file in the dist. Look at the Gruntfile compile task and look into grunt to see how this is achieved.

@chriscoderdr
Copy link

Any update on this?

@dinandmentink
Copy link

Is there an update on this? Really looking forward to this being fixed?

@huseyinbirtan
Copy link

huseyinbirtan commented May 31, 2018

Following modification on select2.js helped for now

c.prototype.highlightFirstItem = function() {
             return false;
},

@boweihan
Copy link
Contributor Author

Let me know if there's anything I can do to help push this through!

@pedrofurtado
Copy link
Contributor

@kevin-brown @alexweissman Any news?

@dinandmentink
Copy link

I have several projects where a monkeypatch is active untill this is released. Is there any estimate on when this will be released? Is there anything I can help with to get this released?

@pedrofurtado
Copy link
Contributor

@dinandmentink I've created a fork of select2 with some monkeypatches of community. See the repo: https://github.com/pedrofurtado/select2

See also the diff: develop...pedrofurtado:develop

@pedrofurtado
Copy link
Contributor

pedrofurtado commented Sep 3, 2018

@dinandmentink Test using this fork to check if the issues are resolved, at least until a new official version is released. I'm using this version in my company's projects.

In your package.json, add the dependency like this below:

{
  "dependencies": {
    "select2": "https://github.com/pedrofurtado/select2#develop"
  }
}

@pedrofurtado pedrofurtado changed the base branch from master to develop September 5, 2018 20:13
@pedrofurtado
Copy link
Contributor

@boweihan I added some docs explaining this new property ( select2/docs@165f945 ).

@pedrofurtado pedrofurtado merged commit 2b049c0 into select2:develop Sep 5, 2018
@arning
Copy link

arning commented Sep 7, 2018

I get following error, when I use the 'develop' version (instead of 4.05):
Uncaught TypeError: Cannot set property 'selected' of undefined

This code causes the error
data.selected = true;
Line 3173 of select2.js

Do I need to configure select2 to make this version work?

@pedrofurtado
Copy link
Contributor

@arning The develop branch is not the best place to use as dependency. You need to use the released versions, like 4.0.5, and so on. Using develop or master of any Github repository you are assuming a risk of some temporary instability.

@arning
Copy link

arning commented Sep 7, 2018

@pedrofurtado
Alright, understood.
Then, I hope, that this issue will be resolved in the next regular release. Since we are using large multi-select dropdown lists, and this behaviour is quite annoying for our users.

jseekamp pushed a commit to jseekamp/select2 that referenced this pull request Dec 12, 2018
…wns to allow toggling of highlightFirstItem() behaviour (select2#5150)

* Add scrollOnSelect as a configurable option

* default scrollOnSelect to true to avoid modifying existing behaviour

* added tests and default option for scrollAfterSelect
@callmejed
Copy link

Cheers this is exactly what I needed. May I point out that you say the configurable option is "scrollOnSelect" while the code you changed in the js is "scrollAfterSelect".

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.

None yet