Skip to content

Including the original event on enter key press and passing it along to final triggers - #5758

Closed
joepetrakovich wants to merge 2 commits into
select2:developfrom
joepetrakovich:keep-original-event-on-other-close-triggers
Closed

Including the original event on enter key press and passing it along to final triggers#5758
joepetrakovich wants to merge 2 commits into
select2:developfrom
joepetrakovich:keep-original-event-on-other-close-triggers

Conversation

@joepetrakovich

Copy link
Copy Markdown
Contributor

This is a continuation of the work done for #5513 by @6pac.

The original event is included if the user tabs off the dropdown, but not if the user uses the enter key to select the item. This change adds the original event to the enter key press and then makes sure it is passed along to both the close and select triggers inside the the results:select handler.

The existing container select handler already includes the original event if it is present, so this is all that was needed to get that to work.

@6pac

6pac commented Jan 30, 2020

Copy link
Copy Markdown
Contributor

Cool. Since Slickgrid is open source, it might be easier to put up a public example for that. I just think it's good to have a concrete example to tease out the subtleties of navigation. I'll try to do that in the next week.

Comment thread src/js/select2/core.js Outdated
evt.preventDefault();
} else if (key === KEYS.ENTER) {
self.trigger('results:select', {});
self.trigger('results:select', evt);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This should be wrapped as originalEvent similar to how we do it for other events.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ah, okay. I wrapped it like that in results.js but thought it would be better to be consistent here with the call to select.close 4 lines above this, and to be consistent with the other handlers in results.js (like the mouseup handler on line 448).

Otherwise the results:select handler will have two originalEvent: **evt**.originalEvent wrappings which are inconsistent with the existing wrapping in the mouseup handler. Seems like trading one inconsistency for two others.

Happy to make the change as long as you're okay with that.

Comment thread src/js/select2/core.js Outdated
evt.preventDefault();
} else if (key === KEYS.ENTER) {
self.trigger('results:select', {});
self.trigger('results:select', evt);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The end goal is to trigger the internal Select2 events in all cases such that they include the triggering jQuery event in the originalEvent key of their parameters. When we relay the events around, it should continue to pass the jQuery event as that key.

My plan was to normalize that within previous changes (as well as this one) if it wasn't done within this pull request. I noticed it was inconsistent previously.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ok cool. That plan sounds great actually.

@stale

stale Bot commented Apr 7, 2020

Copy link
Copy Markdown

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 commented Jul 21, 2020

Copy link
Copy Markdown

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 21, 2020
@stale stale Bot closed this Jul 28, 2020
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.

3 participants