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

feat: add type definitions for change event #3085

Merged
merged 1 commit into from
Nov 19, 2021
Merged

Conversation

web-padawan
Copy link
Member

@web-padawan web-padawan commented Nov 18, 2021

Description

The main idea behind this improvement is providing a type for target property of the change event object.
This is especially important because typically inside a change event listener users check event.target.value.

We might consider updating all the other custom events to have type for target separately later.

Fixes #3040

Type of change

  • Feature

@sonarcloud
Copy link

sonarcloud bot commented Nov 18, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

Copy link
Contributor

@sissbruecker sissbruecker left a comment

Choose a reason for hiding this comment

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

Looks good.

We might consider updating all the other custom events to have type for target separately later.

That would have been my only concern. There is probably an opportunity to add a base event type per component, and use that for all custom events. But I'm not sure if all custom events actually have the host element set as target.

Maybe there is also a language construct where you can modify the value types of HTMLElementEventMap to override the target property 🤔

@web-padawan
Copy link
Member Author

But I'm not sure if all custom events actually have the host element set as target.

Yes, technically they do. I will create a separate issue as a follow-up and we probably can do that in V23.

@web-padawan web-padawan removed the request for review from vursen November 19, 2021 09:03
@web-padawan web-padawan merged commit b3be9ea into master Nov 19, 2021
@web-padawan web-padawan deleted the feat/ts-change-event branch November 19, 2021 09:03
@vaadin-bot
Copy link
Collaborator

Hi @web-padawan , this commit cannot be picked to 22.0 by this bot, can you take a look and pick it manually?
Error Message: Error: Command failed: git cherry-pick b3be9ea
error: could not apply b3be9ea... feat: add type definitions for change event (#3085)
hint: after resolving the conflicts, mark the corrected paths
hint: with 'git add ' or 'git rm '
hint: and commit the result with 'git commit'

@web-padawan
Copy link
Member Author

We probably should not pick this to Vaadin 22 as we are about to have a release candidate today.

@vaadin-bot
Copy link
Collaborator

This ticket/PR has been released with platform 23.0.0.

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.

Add TypeScript definition for change event [⏳ 1d]
3 participants