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 support for @outside global event filter #695

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lb-
Copy link
Contributor

@lb- lb- commented Jun 20, 2023

@lb-
Copy link
Contributor Author

lb- commented Jun 20, 2023

Important: This has been built to consider the Controlled element as what should be the boundary, not sure on the best way to go about this. We could change this to be the bounds of the element that has the data-action to be more flexible.

For example, these will both behave the same, only clicks that are 'fully outside' the controlled element will call a#something.

<main>
  <div id="outer" data-controller="a" data-action="click@outside->a#something">
    <button type="button" id="inner-button">outer<button>
    <div id="inner" data-action="click@outside->a#something">
      <button type="button" id="inner-button">inner</button>
    </div>
  </div>
  <button type="button">fully outside</button>
</main>

This may not be intuitive, so happy to adjust the PR so that it works based on the contains being compared against the element with the data-action.

src/core/binding.ts Outdated Show resolved Hide resolved
@adrienpoly
Copy link
Member

This may not be intuitive, so happy to adjust the PR so that it works based on the contains being compared against the element with the data-action.

That would feel more natural to me having the outside being compared to the element with the data action and not the controller by itself.

@lb- lb- force-pushed the feature/656-add-outside-global-event-filter branch 2 times, most recently from db64cb5 to 7069d82 Compare June 21, 2023 07:54
@lb-
Copy link
Contributor Author

lb- commented Jun 21, 2023

@adrienpoly yeah, I had a think about this and agree it would be pretty confusing, I have now changed the behaviour and updated unit tests.

When you set the data-action, that will be the 'contains' element, irrespective of the controlled element.

@lb- lb- force-pushed the feature/656-add-outside-global-event-filter branch from 7069d82 to f919a43 Compare June 21, 2023 08:14
@lb-
Copy link
Contributor Author

lb- commented Jun 21, 2023

Not sure why it's failing, I don't think it's related to this PR.

Safari 14.1 (Mac OS 10.15.7) EventOptionsTests custom option FAILED
	Expected: 2
	Actual: 1
	
	step
	fulfilled
	promiseReactionJob@[native code]
	
	Expected: {
	  "eventType": "toggle",
	  "name": "log"
	}
	Actual: {
	  "eventType": undefined,
	  "name": undefined
	}
	
	
	step
	fulfilled
	promiseReactionJob@[native code]
	

@dhh dhh requested a review from marcoroth June 24, 2023 11:23
@lb- lb- force-pushed the feature/656-add-outside-global-event-filter branch from f919a43 to 67b7df1 Compare June 26, 2023 21:49
@lb-
Copy link
Contributor Author

lb- commented Jun 26, 2023

Rebased

@lb- lb- force-pushed the feature/656-add-outside-global-event-filter branch from 67b7df1 to 1460534 Compare August 20, 2023 21:42
@lb-
Copy link
Contributor Author

lb- commented Aug 20, 2023

Rebased

@tonysm
Copy link

tonysm commented Nov 25, 2023

Nice work. I'd love to see this being merged! I was about to attempt implementing this after adding the same closeWhenClickedOutside action to different controllers.

- When using `@outside`, it will behave the same as @document but only trigger the action if the event was triggered from outside the element with the attached action
- Closes hotwired#656
@lb- lb- force-pushed the feature/656-add-outside-global-event-filter branch from 1460534 to 46bbce1 Compare February 7, 2024 23:03
@lb-
Copy link
Contributor Author

lb- commented Feb 7, 2024

Just rebased

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Feature request - Action option or global handling of 'outside' events
3 participants