Skip to content

Fixups: post-merge updates related to PR #6241#6335

Merged
kevin-brown merged 6 commits into
select2:developfrom
openculinary:pr-6241-followup/qunit-access-fixup
Aug 24, 2024
Merged

Fixups: post-merge updates related to PR #6241#6335
kevin-brown merged 6 commits into
select2:developfrom
openculinary:pr-6241-followup/qunit-access-fixup

Conversation

@jayaddison
Copy link
Copy Markdown
Contributor

@jayaddison jayaddison commented Aug 23, 2024

This pull request includes a

  • Bug fix
  • New feature
  • Translation

The following changes were made

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

Relates-to / merge-resolution-for #6241 and #6334.

Edit: add additional fixup descriptions.

@jayaddison

This comment was marked as outdated.

@jayaddison

This comment was marked as outdated.

@jayaddison

This comment was marked as outdated.

@jayaddison jayaddison changed the title Fixup: reference QUnit.test instead of relying on global namespace entry Fixups: post-merge updates related to PR #6241 Aug 24, 2024
@jayaddison jayaddison marked this pull request as draft August 24, 2024 11:23
@jayaddison
Copy link
Copy Markdown
Contributor Author

@kevin-brown something about the searching tags does not loose focus integration test seems incompatible with #6241 -- I've commented it out here, and that allows the test suite to pass, but it's not an approach I like so I've moved this into draft status until the problem is figured out.

@jayaddison
Copy link
Copy Markdown
Contributor Author

The problem seems to be that selection:update event handling in the integration tests isn't isolated per-test-case. Some ideas to resolve this:

  • Add some kind of per-test teardown that removes relevant event handling/selection-update resources (listeners, DOM contents, internal state?).
  • Attach once-only event handlers in each of the relevant integration test cases.
  • Add logic to ensure that each event-handler becomes essentially a no-op despite remaining attached during unrelated integration tests.

I attempted to add a QUnit.reset handler from here to achieve the first; but this did not seem to work.

The second approach doesn't appear straightforward either; there is no .once(...) function available for event-listener attachment on Select2 instances, nor is there a way to pass {once: true} as a parameter to .on(...).

...so I've slightly-reluctantly implemented the third approach. It doesn't genuinely provide any meaningful runtime isolation between the integration test cases, but it ensure that each event listener should only perform any actions within the scope of its parent test case.

@jayaddison jayaddison marked this pull request as ready for review August 24, 2024 14:36
@jayaddison
Copy link
Copy Markdown
Contributor Author

@kevin-brown it's not ideal code (details in my previous message if you're curious), but I believe this is ready for merge.

@kevin-brown
Copy link
Copy Markdown
Member

I'll take the "temporary" hack for now. Thanks for looking into this and documenting your findings!

@kevin-brown kevin-brown merged commit 07bd61a into select2:develop Aug 24, 2024
@jayaddison jayaddison deleted the pr-6241-followup/qunit-access-fixup branch August 24, 2024 22:12
kevin-brown added a commit that referenced this pull request May 26, 2026
- Remove '(unreleased)' marker from the 4.1.0 heading
- Add new features: jQuery 4.0.0 support (#6332), originalEvent in
  close trigger args (#6079)
- Add bug fixes: placeholder misalignment (#6277), RTL choice remove
  button (#6257), digit-only data-placeholder (#6297), AJAX unselection
  with non-string IDs (#6241, #6335), optgroup child string coercion (#6338)
- Add translations: lb (#6131), ug (#6166), ar (#6175), zh-TW (#6157),
  id (#6153), tr (#6123), ro (#6190), de/es/fr/pt/pt-BR (#6132),
  pl (#6097, #6377), nb (#6213), fa (#6258), nl (#6269), pt-BR typo
  (#6200), missing bs/ca/da/fi strings (#6305)
- Add miscellaneous: native DOM replacements for jQuery attr/removeAttr
  (#6227, #6228), classList.add (#6229), jQuery removal from Utils and
  Translation (#6233), prop() removal (#6289), NPM trusted publishing
  (#6405)
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.

2 participants