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 KeyboardMixin and make other mixins use it #2432

Merged
merged 7 commits into from
Sep 3, 2021

Conversation

vursen
Copy link
Contributor

@vursen vursen commented Sep 1, 2021

Description

The ActiveMixin, ClearButtonMixin and future CheckedMixin all need to handle the keyup and keydown events.

Along with that, the prototype of the new Checkbox component supposes it to inherit both ActiveMixin and CheckedMixin. This being so, these mixins will end up in the same scope and there may happen a naming collision (in case both define a _onKeyDown method, for example).

So it was decided to introduce KeyboardMixin that is responsible for subscribing to the keyboard events while leaving actual implementation of the event handlers to the client (a component or another mixin).

See the prototype: https://github.com/vaadin/component-mixins/blob/master/packages/keyboard-mixin/keyboard-mixin.ts

Part of #2210.

Type of change

  • Feature

Checklist

  • I have read the contribution guide: https://vaadin.com/docs-beta/latest/guide/contributing/overview/
  • I have added a description following the guideline.
  • The issue is created in the corresponding repository and I have referenced it.
  • I have added tests to ensure my change is effective and works as intended.
  • New and existing tests are passing locally with my change.
  • I have performed self-review and corrected misspellings.

packages/field-base/src/active-mixin.js Outdated Show resolved Hide resolved
packages/field-base/src/keyboard-mixin.d.ts Show resolved Hide resolved
packages/field-base/src/keyboard-mixin.d.ts Outdated Show resolved Hide resolved
packages/field-base/src/keyboard-mixin.d.ts Outdated Show resolved Hide resolved
packages/field-base/src/keyboard-mixin.js Show resolved Hide resolved
packages/field-base/test/keyboard-mixin.test.js Outdated Show resolved Hide resolved
Co-authored-by: Serhii Kulykov <iamkulykov@gmail.com>
@vursen vursen force-pushed the feat/keyboard-mixin branch from 9b317c6 to 723e5bf Compare September 2, 2021 14:10
@vursen vursen force-pushed the feat/keyboard-mixin branch from 723e5bf to d5ea194 Compare September 2, 2021 14:11
@web-padawan web-padawan changed the title feat: add KeyboardMixin feat: add KeyboardMixin and make other mixins use it Sep 2, 2021
Copy link
Member

@web-padawan web-padawan left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@sonarqubecloud
Copy link

sonarqubecloud bot commented Sep 3, 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

@vaadin-bot
Copy link
Collaborator

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

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