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

refactor!: update date-picker to use slotted input #2532

Merged
merged 40 commits into from
Sep 17, 2021
Merged

refactor!: update date-picker to use slotted input #2532

merged 40 commits into from
Sep 17, 2021

Conversation

tomivirkki
Copy link
Member

@tomivirkki tomivirkki commented Sep 14, 2021 •
edited
Loading

Description

Fixes #2204

Type of change

  • Breaking change

@tomivirkki tomivirkki marked this pull request as draft September 14, 2021 14:16
@tomivirkki tomivirkki force-pushed the refactor/date-picker-slotted branch 5 times, most recently from 8f6b174 to 0ab2cd3 Compare September 15, 2021 12:11
@nc142j
Copy link

nc142j commented Sep 15, 2021

Hi @tomivirkki , do you know if this update will fix #4168?

@tomivirkki
Copy link
Member Author

Hi @tomivirkki , do you know if this update will fix #4168?

Hi @nc142j this will probably not change how date-time-picker works. Added a comment to the issue.

@tomivirkki tomivirkki force-pushed the refactor/date-picker-slotted branch 2 times, most recently from 3af8a08 to dadc770 Compare September 16, 2021 08:59
web-padawan
web-padawan requested changes Sep 17, 2021
View reviewed changes
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.

Mostly minor comments. I still have to check the screenshots to see what has changed.

packages/vaadin-date-picker/src/vaadin-date-picker-mixin.d.ts Show resolved Hide resolved
packages/vaadin-date-picker/src/vaadin-date-picker-mixin.d.ts Outdated Show resolved Hide resolved
packages/vaadin-date-picker/src/vaadin-date-picker-mixin.js Outdated Show resolved Hide resolved
packages/vaadin-date-picker/src/vaadin-date-picker-mixin.js Outdated Show resolved Hide resolved
packages/vaadin-date-picker/src/vaadin-date-picker-mixin.js Outdated Show resolved Hide resolved
packages/vaadin-date-picker/src/vaadin-date-picker.js Outdated Show resolved Hide resolved
packages/vaadin-date-picker/test/basic.test.js Outdated Show resolved Hide resolved
packages/vaadin-date-picker/test/basic.test.js Outdated Show resolved Hide resolved
packages/vaadin-date-picker/test/validation.test.js Outdated Show resolved Hide resolved
packages/vaadin-date-picker/test/wai-aria.test.js Outdated Show resolved Hide resolved
web-padawan
web-padawan requested changes Sep 17, 2021
View reviewed changes
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.

Please fix the corner of the date-picker when used in vaadin-date-time-picker:

corner

web-padawan
web-padawan reviewed Sep 17, 2021
View reviewed changes
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.

A few more suggestions, sorry I did not cover all the files in one review round 😓

packages/vaadin-date-picker/src/vaadin-date-picker-mixin.js Outdated Show resolved Hide resolved
packages/vaadin-date-picker/src/vaadin-date-picker.d.ts Outdated Show resolved Hide resolved
packages/vaadin-date-picker/src/vaadin-date-picker-mixin.js Show resolved Hide resolved
@vursen vursen mentioned this pull request Sep 17, 2021
Merged
13 tasks
@web-padawan web-padawan mentioned this pull request Sep 17, 2021
Merged
@sonarcloud
Copy link

sonarcloud bot commented Sep 17, 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 10 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

web-padawan
web-padawan reviewed Sep 17, 2021
View reviewed changes
packages/vaadin-date-picker/theme/lumo/vaadin-date-picker-styles.js
Comment on lines +25 to +26
/* TODO: fix this in input-container, update all screenshots */
-webkit-mask-image: var(--_lumo-text-field-overflow-mask-image);
Copy link
Member

Choose a reason for hiding this comment

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

It turns out mask-image still needs a prefix in latest Chrome 🙈 This explains why I had to update screenshots for new field components as the difference was really tricky to spot.

Added this to preserve correct screenshots for the date-picker and avoid updating them in this PR.
I will restore prefixed property in vaadin-input-container in other PR, and add RTL visual tests.

@web-padawan web-padawan merged commit e67a1a1 into master Sep 17, 2021
@web-padawan web-padawan deleted the refactor/date-picker-slotted branch September 17, 2021 19:59
This was referenced Sep 18, 2021
Closed
Closed
Merged
@tomivirkki
Copy link
Member Author

There's a branch with necessary changes to DatePicker / DateTimePicker that needs to be merged together with the Web Component version bump.

@vaadin-bot
Copy link
Collaborator

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

@web-padawan web-padawan mentioned this pull request Dec 24, 2021
Closed
@web-padawan web-padawan mentioned this pull request Sep 6, 2022
Merged
@web-padawan web-padawan mentioned this pull request Oct 23, 2022
Merged
@vursen vursen mentioned this pull request Aug 11, 2023
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@web-padawan web-padawan web-padawan approved these changes

Assignees
No one assigned
Labels
a11y Accessibility issue prerelease version for platform 22.0.0 Released with Platform 22.0.0.alpha7
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Re-implement date-picker using slotted input and new mixins
4 participants
@tomivirkki @nc142j @vaadin-bot @web-padawan