-
-
Notifications
You must be signed in to change notification settings - Fork 719
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
chore(deps): bump tom-select from 2.2.3 to 2.3.1 #11759
Conversation
Some of the changes broke some integration test that are using |
0b2fe63
to
d01d4b3
Compare
I'm stuck and not sure how to continue. My notes so far.. I had a look at the customer selector on admin orders page, and can't work out what's wrong yet. That one uses a specific Stimulus controller:
The problem is with loading remote data.
Upgrade clues:Comparing orchidjs/tom-select@v2.2.3...v2.3.0
Issues: https://github.com/orchidjs/tom-select/issuesjust one recent one open, but no obvious connection. |
Fixed the problem by using I also found a bug where customer search by first name doesn't work when creating a new order, it's fixed here #12015 and will need to be merged before we can merge this. |
67695de
to
4598c7c
Compare
Bumps [tom-select](https://github.com/orchidjs/tom-select) from 2.2.3 to 2.3.1. - [Release notes](https://github.com/orchidjs/tom-select/releases) - [Commits](orchidjs/tom-select@v2.2.3...v2.3.1) --- updated-dependencies: - dependency-name: tom-select dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com>
Setting the value directly doesn't trigger the search, presumably because it doesn't trigger any javascript event.
4598c7c
to
270a310
Compare
When using `tomselect_search_and_select` and searching isn't really required it leaves the dropdown option open. It can then cause problem when trying to interact with other element in the page. This happens because clicking on the chosen option happena before the searching finishes. We can now use `tomselect_select` when searching is not actually required. It should not be a problem when search is required, as capybara will wait for the option to appear on the page before clicking.
In these scenarios, searching for the option is not actually required, we can directly click on the needed option. It prevent issue with the dropdown option staying open and breaking specs.
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.
Great!
I wonder in which case the browser would use set. Maybe when prefilling an input with historical data? That's probably not needed with a search input field but one could consider that a bug in tomselect. I don't think we care though. Or would accessibility aids like speech-to-text use set?
def tomselect_select(value, options) | ||
tomselect_wrapper = page.find("[name='#{options[:from]}']").sibling(".ts-wrapper") |
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.
It may be worth being a bit more explicit with the options:
def tomselect_select(value, options) | |
tomselect_wrapper = page.find("[name='#{options[:from]}']").sibling(".ts-wrapper") | |
def tomselect_select(value, from:) | |
tomselect_wrapper = page.find("[name='#{from}']").sibling(".ts-wrapper") |
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.
Great, thanks for fixing this one!
Maikel's suggestion looks good too, it could be applied to the other methods in that file also if you have the time.
Regarding the issue, am I right in saying it's a race condition, due to the input being filled in too early? In that case it sounds like a bug indeed. |
I just remembered that Gaetan won't be online this week, so I think better just to proceed as-is. Thanks to the testing, I think we can assume there's no regressions and merge without manual testing. |
It's a race condition, but due to clicking on an option before the search actually finished. There is nothing really telling us the search is still happening, but a user is never as quick as Capybara so it's not really an issue. I guess it could be considered a bug. |
Bumps tom-select from 2.2.3 to 2.3.1.
Release notes
Sourced from tom-select's releases.
Commits
69180fa
v2.3.1 Release2eecbfe
rgba() instead of rgb(). Fixes #647b02c3ce
"include relevant error messages"6473bdd
Merge branch 'master' of github.com:orchidjs/tom-selectdc7a1a6
HTMLSelectElement doesn't have readOnly attribute. Fixes #650df86451
Merge pull request #649 from ravage84/patch-1190a21b
Update copyright year6d11820
v2.3.0 Releasedceef1c
remove jquery7bd39ea
remove deprecated hideInput() and showInput()You can trigger a rebase of this PR by commenting
@dependabot rebase
.Dependabot commands and options
You can trigger Dependabot actions by commenting on this PR:
@dependabot rebase
will rebase this PR@dependabot recreate
will recreate this PR, overwriting any edits that have been made to it@dependabot merge
will merge this PR after your CI passes on it@dependabot squash and merge
will squash and merge this PR after your CI passes on it@dependabot cancel merge
will cancel a previously requested merge and block automerging@dependabot reopen
will reopen this PR if it is closed@dependabot close
will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually@dependabot show <dependency name> ignore conditions
will show all of the ignore conditions of the specified dependency@dependabot ignore this major version
will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)@dependabot ignore this minor version
will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)@dependabot ignore this dependency
will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)