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.

Sign up for GitHub

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

Jump to bottom

feat: introduce event.target element to DOM event #11992

Merged
merged 2 commits into from Oct 11, 2021
Merged

feat: introduce event.target element to DOM event #11992

merged 2 commits into from Oct 11, 2021

Conversation

pleku
Copy link
Contributor

@pleku pleku commented Oct 7, 2021 •
edited

Makes it possible to obtain the Element instance on the server side
that corresponds to event.target for a DOM event. The mapping needs
to be explicitly enabled with DomListenerRegistration.mapEventTargetElement()
for it to work.

Next steps would be to introduce an API to certain component events that
leverage the API, or generalizing the API so that one could give out an
JS string that would be mapped instead of event.target.

Closes #3356

This was referenced Oct 7, 2021
Open
Closed
@caalador caalador self-requested a review October 7, 2021 07:03
caalador
caalador reviewed Oct 7, 2021
View reviewed changes
Copy link
Contributor

@caalador caalador left a comment

Choose a reason for hiding this comment

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

Seems fine to me, but I'll leave it up to @mstahv to decide when this would be taken in.

@caalador
Copy link
Contributor

caalador commented Oct 7, 2021

one test is failing:

DomApiAbstractionUsageTest.testDomApiCodeNotUsedcom.vaadin.client

java.lang.AssertionError: com.vaadin.client.flow.binding.SimpleElementBindingStrategy.getClosestStateNodeToTarget calls elemental.dom.Node.isEqualNode
  at org.junit.Assert.fail(Assert.java:89)
  at com.vaadin.client.DomApiAbstractionUsageTest.verifyMethod(DomApiAbstractionUsageTest.java:148)
  at com.vaadin.client.DomApiAbstractionUsageTest.access$300(DomApiAbstractionUsageTest.java:36)
  at com.vaadin.client.DomApiAbstractionUsageTest$1$1.visitMethodInsn(DomApiAbstractionUsageTest.java:98)

@mstahv
Copy link
Member

mstahv commented Oct 7, 2021

Looks like a needed feature to me, I think I would have needed this when doing one add-on in the past as well.

Check if we have a natural place to discuss about this enhancement in our docs. My guess is not, and if so, let's just leave it with JavaDocs only. There is also other low level DOM API that is only covered in JavaDocs.

@knoobie
Copy link
Contributor

knoobie commented Oct 7, 2021

I really like the addition; just one question - what do you think about renaming mapEventTargetToElement to withEventTarget, withEventTargetAsElement or withMappedEventTarget? Something starting with withto show that this is some kind of builder pattern?

@pleku
feat: introduce event.target element to DOM event
1da1c23 
Makes is possible to obtain the Element instance on the server side
that corresponds to `event.target` for a DOM event. The mapping needs
to be explicitly enabled with DomListenerRegistration.mapEventTargetElement()
for it to work.

Next steps would be to introduce an API to certain component events that
leverage the API, or generalizing the API so that one could give out an
JS string that would be mapped instead of `event.target`.

Closes #3356
@pleku
Copy link
Contributor Author

pleku commented Oct 10, 2021

what do you think about renaming mapEventTargetToElement to withEventTarget, withEventTargetAsElement or withMappedEventTarget? Something starting with withto show that this is some kind of builder pattern?

None of the existing API in the class follow this pattern e.g. synchronizeProperty instead of withSynchronizedProperty or debounce ... so while I would like the idea, it feels to me like to keep it consistent, it should start with the specific verb in this case, which for me is map. So to change it to with, it should be a refactor to change everything instead (in my opinion).

@pleku pleku enabled auto-merge (squash) October 10, 2021 13:02
@vaadin-bot
Copy link
Collaborator

SonarQube analysis reported 2 issues

  1. CRITICAL SimpleElementBindingStrategy.java#L1468: Refactor this code to not nest more than 3 if/for/while/switch/try statements. rule
  2. CRITICAL SimpleElementBindingStrategy.java#L1474: Either log or rethrow this exception. rule

@pleku pleku added +0.1.0 and removed +1.0.0 labels Oct 10, 2021
@pleku
Copy link
Contributor Author

pleku commented Oct 10, 2021

The bot is mistaken as the added interface method has a default implementation and is not thus a breaking change.

@pleku pleku merged commit 239c104 into master Oct 11, 2021
OLD Vaadin Flow ongoing work (Vaadin 10+) automation moved this from Iteration Reviews to Done - pending release Oct 11, 2021
@pleku pleku deleted the feat/3356 branch October 11, 2021 11:53
@vaadin-bot
Copy link
Collaborator

This ticket/PR has been released with platform 22.0.0.alpha8 and is also targeting the upcoming stable 22.0.0 version.

pleku added a commit that referenced this pull request Nov 11, 2021
@pleku
feat: add support for @eventdata of type Component
9dad86a 
Makes it possible to use event data expressions that map to any Component / Element from server side state tree.
For example it will be possible to use:
- `@EventData("event.target") Component eventTarget` for `@DomEvent`
- `@EventData("event.target") Element eventTarget` for `@DomEvent`
- `DomEventRegistration::addEventDataElement(String expression)` to get any element mapped,
- and fetch it with `DomEvent::getEventDataElement(String expression)

Follow up for #11992, #3356
@pleku pleku mentioned this pull request Nov 11, 2021
Merged
joheriks pushed a commit that referenced this pull request Nov 18, 2021
@pleku
feat: add support for @eventdata of type Component (#12334)
29d99a8 
Makes it possible to use event data expressions that map to any Component / Element from server side state tree.
For example it will be possible to use:
- `@EventData("event.target") Component eventTarget` for `@DomEvent`
- `@EventData("event.target") Element eventTarget` for `@DomEvent`
- `DomEventRegistration::addEventDataElement(String expression)` to get any element mapped,
- and fetch it with `DomEvent::getEventDataElement(String expression)

Follow up for #11992, #3356
pleku added a commit that referenced this pull request Nov 19, 2021
@pleku
feat: introduce event.target element to DOM event (#11992)
964e659 
Makes is possible to obtain the Element instance on the server side
that corresponds to `event.target` for a DOM event. The mapping needs
to be explicitly enabled with DomListenerRegistration.mapEventTargetElement()
for it to work.

Next steps would be to introduce an API to certain component events that
leverage the API, or generalizing the API so that one could give out an
JS string that would be mapped instead of `event.target`.

Closes #3356
pleku added a commit that referenced this pull request Nov 19, 2021
@pleku
feat: add support for @eventdata of type Component (#12334)
98fed95 
Makes it possible to use event data expressions that map to any Component / Element from server side state tree.
For example it will be possible to use:
- `@EventData("event.target") Component eventTarget` for `@DomEvent`
- `@EventData("event.target") Element eventTarget` for `@DomEvent`
- `DomEventRegistration::addEventDataElement(String expression)` to get any element mapped,
- and fetch it with `DomEvent::getEventDataElement(String expression)

Follow up for #11992, #3356
pleku added a commit that referenced this pull request Nov 19, 2021
@pleku
feat: introduce event.target element to DOM event (#11992)
3bb17a7 
Makes is possible to obtain the Element instance on the server side
that corresponds to `event.target` for a DOM event. The mapping needs
to be explicitly enabled with DomListenerRegistration.mapEventTargetElement()
for it to work.

Next steps would be to introduce an API to certain component events that
leverage the API, or generalizing the API so that one could give out an
JS string that would be mapped instead of `event.target`.

Closes #3356
pleku added a commit that referenced this pull request Nov 19, 2021
@pleku
feat: add support for @eventdata of type Component (#12334)
76660de 
Makes it possible to use event data expressions that map to any Component / Element from server side state tree.
For example it will be possible to use:
- `@EventData("event.target") Component eventTarget` for `@DomEvent`
- `@EventData("event.target") Element eventTarget` for `@DomEvent`
- `DomEventRegistration::addEventDataElement(String expression)` to get any element mapped,
- and fetch it with `DomEvent::getEventDataElement(String expression)

Follow up for #11992, #3356
pleku added a commit that referenced this pull request Nov 29, 2021
@pleku
feat: introduce event.target element to DOM event (#11992)
ca1a1f6 
Makes is possible to obtain the Element instance on the server side
that corresponds to `event.target` for a DOM event. The mapping needs
to be explicitly enabled with DomListenerRegistration.mapEventTargetElement()
for it to work.

Next steps would be to introduce an API to certain component events that
leverage the API, or generalizing the API so that one could give out an
JS string that would be mapped instead of `event.target`.

Closes #3356
pleku added a commit that referenced this pull request Nov 29, 2021
@pleku
feat: add support for @eventdata of type Component (#12334)
200ae96 
Makes it possible to use event data expressions that map to any Component / Element from server side state tree.
For example it will be possible to use:
- `@EventData("event.target") Component eventTarget` for `@DomEvent`
- `@EventData("event.target") Element eventTarget` for `@DomEvent`
- `DomEventRegistration::addEventDataElement(String expression)` to get any element mapped,
- and fetch it with `DomEvent::getEventDataElement(String expression)

Follow up for #11992, #3356
@vaadin-bot
Copy link
Collaborator

This ticket/PR has been released with Vaadin 14.9.0.alpha1 and is also targeting the upcoming stable 14.9.0 version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@caalador caalador caalador approved these changes

Assignees
No one assigned
Labels
prerelease version for platform 22.0.0 prerelease version for Vaadin 14.9.0 Released with Platform 22.0.0.alpha8 Released with Vaadin 14.9.0.alpha1 +0.1.0
Projects
OLD Vaadin Flow ongoing work (Vaadin ...
  
Done - pending release
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make it possible to get event.target element on the server side
5 participants
@pleku @caalador @mstahv @knoobie @vaadin-bot