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

Set clear button not opening the overlay #620

Merged
merged 3 commits into from
Feb 28, 2018
Merged

Conversation

yuriy-fix
Copy link
Contributor

@yuriy-fix yuriy-fix commented Feb 27, 2018

Fixes #613

Not sure if I can properly test the focused state of the input on mobile and desktop. Will be grateful for any suggestions.


This change is Reviewable

@@ -90,6 +91,8 @@
_item-label-path="[[itemLabelPath]]"
loading="[[loading]]">
</vaadin-combo-box-dropdown-wrapper>

<iron-media-query query="(min-width: 600px)" query-matches="{{_desktopMode}}"></iron-media-query>
Copy link
Member

Choose a reason for hiding this comment

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

Device width is not a really detecting the input method, is it? :)

I don’t like the fact that we need a new dependency for this (iron-media-query). We should detect touch devices, whether the button was clicked with a finger or a touch device.

@web-padawan
Copy link
Member

Reviewed 5 of 5 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


src/vaadin-combo-box.html, line 95 at r1 (raw file):

Previously, jouni (Jouni Koivuviita) wrote…

Device width is not a really detecting the input method, is it? :)

I don’t like the fact that we need a new dependency for this (iron-media-query). We should detect touch devices, whether the button was clicked with a finger or a touch device.

BTW, we use 420px in dropdown-menu and date-piker, and also 750px for "wide" in context-menu. If we are not inclined to re-use our device detector as a separate element or mixin, let's at least align the numbers.


Comments from Reviewable

@yuriy-fix
Copy link
Contributor Author

Review status: 2 of 3 files reviewed at latest revision, 1 unresolved discussion.


src/vaadin-combo-box.html, line 95 at r1 (raw file):

Previously, web-padawan (Sergey Kulikov) wrote…

BTW, we use 420px in dropdown-menu and date-piker, and also 750px for "wide" in context-menu. If we are not inclined to re-use our device detector as a separate element or mixin, let's at least align the numbers.

I have removed media-query and now using another way of detection by checking ontouchstart property. Is it better or should I try to find other solution?


Comments from Reviewable

@web-padawan
Copy link
Member

:lgtm:


Reviewed 2 of 3 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

@tomivirkki
Copy link
Member

:lgtm:


Reviewed 2 of 5 files at r1, 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

@jouni
Copy link
Member

jouni commented Feb 27, 2018

The vaadin-device-detector in context-menu is a rather odd one. I feel like we should get rid of that, or at least extract it out as a separate utility mixin.

@web-padawan
Copy link
Member

@jouni +1 to the mixin, at least to detect touch device / IE / Safari etc. Something like DeviceAwareMixin re-used wherever we need it.

@tomivirkki
Copy link
Member

Reviewed 1 of 1 files at r4.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

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

Successfully merging this pull request may close these issues.

4 participants