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

Add focus and blur methods, focused property #398

Merged
merged 1 commit into from Feb 14, 2017
Merged

Conversation

platosha
Copy link
Contributor

@platosha platosha commented Feb 9, 2017

Fixes #349


This change is Reviewable

@limonte
Copy link
Contributor

limonte commented Feb 10, 2017

:lgtm:


Review status: 0 of 2 files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@manolo
Copy link
Member

manolo commented Feb 10, 2017

I miss the tabindex property that should have any form control. But after experimenting with it, I don't found a 100% working solution. So let's leave this for a separated PR because other elements also have the same problem.

:lgtm:


Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@tomivirkki
Copy link
Member

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


vaadin-combo-box.html, line 407 at r1 (raw file):

    },

    _onInputFocus: function() {

This isn't used


test/vaadin-combo-box-properties.html, line 209 at r1 (raw file):

        it('should focus the input with focus method', function() {
          var inputFocusCalled = false;

Unneeded variable. sinon.stub returns an object whose called property can be asserted. These could also be moved to beforeEach since they're used in 2 tests


Comments from Reviewable

@Saulis
Copy link
Contributor

Saulis commented Feb 10, 2017

I too think this should work with tabbing. Here's a prototype proposal that enables tabbing both in Shadow and Shady: https://jsfiddle.net/Saulis/8mj7xzfa/


Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


Comments from Reviewable

@manolo
Copy link
Member

manolo commented Feb 13, 2017

Smart solution @Saulis, I added it to my tabindex conclusions here:
https://github.com/vaadin/vaadin-button/blob/proto/native-button/tabindex-research.md


Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


Comments from Reviewable

@platosha
Copy link
Contributor Author

@Saulis is it ok to add this in a separate PR?


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


vaadin-combo-box.html, line 407 at r1 (raw file):

Previously, tomivirkki (Tomi Virkki) wrote…

This isn't used

Done.


test/vaadin-combo-box-properties.html, line 209 at r1 (raw file):

Previously, tomivirkki (Tomi Virkki) wrote…

Unneeded variable. sinon.stub returns an object whose called property can be asserted. These could also be moved to beforeEach since they're used in 2 tests

Done.


Comments from Reviewable

@Saulis
Copy link
Contributor

Saulis commented Feb 14, 2017

Yes I'm OK with that, I can add a separate ticket for it.


Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


Comments from Reviewable

@tomivirkki
Copy link
Member

:lgtm:


Reviewed 2 of 2 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@tomivirkki tomivirkki merged commit 96d79df into master Feb 14, 2017
@tomivirkki tomivirkki deleted the fix/focus-api branch February 14, 2017 09:47
manolo pushed a commit to vaadin/vaadin-combo-box-flow that referenced this pull request Oct 3, 2020
manolo pushed a commit to vaadin/vaadin-combo-box-flow that referenced this pull request Oct 3, 2020
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.

None yet

5 participants