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

vaadin-number-field element #110

Merged
merged 14 commits into from
Jan 16, 2019
Merged

vaadin-number-field element #110

merged 14 commits into from
Jan 16, 2019

Conversation

gatanaso
Copy link
Contributor

@gatanaso gatanaso commented Aug 8, 2017

Connects to #296

This element is a specification over the vaadin-text-field element that allows numeric value input. The vaadin-number-field element can be used as a simple input element with decrease/increase buttons to update the value. Additionally, it supports specifying min/max input value limits. For more details, please refer to the documentation and demo pages of the component.


This change is Reviewable

@CLAassistant
Copy link

CLAassistant commented Aug 8, 2017

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@samiheikki samiheikki left a comment

Choose a reason for hiding this comment

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

Great code, comments and test coverage, really good! I left a couple of comments that I saw. But we'll have to discuss about the API/designs in a later point, before we can merge/give additional feedback.

demo/number-field.html Outdated Show resolved Hide resolved
test/visual/number-field.html Outdated Show resolved Hide resolved
Copy link
Contributor

@limonte limonte left a comment

Choose a reason for hiding this comment

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

A small thing to change, otherwise nicely done!

vaadin-number-field.html Outdated Show resolved Hide resolved
Copy link
Contributor

@abdonrd abdonrd left a comment

Choose a reason for hiding this comment

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

The controls works even though the input is disabled.

@gatanaso
Copy link
Contributor Author

@abdonrd Thank you very much for the suggestion. Latest commit fixes that issue.

@abdonrd
Copy link
Contributor

abdonrd commented Aug 16, 2017

There are any ETA to publish this element?

@gatanaso gatanaso self-assigned this Aug 17, 2017
@jouni
Copy link
Member

jouni commented Aug 28, 2017

There are any ETA to publish this element?

Not yet, sorry. But I think we will merge and tag a version within the next month. It might not be stable at that point, but hopefully beta quality at least.

vaadin-number-field.html Outdated Show resolved Hide resolved
@limonte limonte force-pushed the feature/vaadin-number-field branch from d74ad8d to f9cc0fd Compare August 28, 2017 16:13
@limonte limonte assigned limonte and unassigned gatanaso Aug 28, 2017
@gatanaso
Copy link
Contributor Author

vaadin-number-field.html, line 40 at r6 (raw file):

Previously, limonte (Limon Monte) wrote…

Done, thanks!

Thanks for the info @jouni :)
And thanks @limonte for making the update!


Comments from Reviewable

@tomivirkki tomivirkki removed the ⭐️ label Sep 4, 2017
@manolo manolo added backlog and removed backlog labels Sep 8, 2017
@tomivirkki tomivirkki removed the ⭐️ label Sep 8, 2017
@manolo
Copy link
Member

manolo commented Sep 8, 2017

when clicking on control buttons, the component is focused (out line is shown) but not the native input, so you cannot write or use arrow keys.


Review status: 0 of 12 files reviewed at latest revision, 5 unresolved discussions, some commit checks broke.


vaadin-number-field.html, line 97 at r7 (raw file):

          return {
            /**
            * Set to true to display value increase/decrease controls.

nit, jsdoc needs that you ident one space the second line and following ones


Comments from Reviewable

@manolo
Copy link
Member

manolo commented Sep 8, 2017

Needs rebasing and pass CI


Review status: 0 of 12 files reviewed at latest revision, 5 unresolved discussions, some commit checks broke.


Comments from Reviewable

@manolo manolo added enhancement New feature or request requires user testing labels Sep 8, 2017
vaadin-number-field.html Outdated Show resolved Hide resolved
Copy link
Member

@tomivirkki tomivirkki left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 16 files at r9, 19 of 23 files at r10, 4 of 4 files at r11.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @samiheikki, @limonte, @jouni, @gatanaso, and @sudipta1411)

Copy link
Member

@manolo manolo left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 16 files at r9, 17 of 23 files at r10, 4 of 4 files at r11.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @samiheikki, @limonte, @jouni, @gatanaso, and @sudipta1411)

Copy link
Contributor

@samiheikki samiheikki left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 16 files at r9, 19 of 23 files at r10, 4 of 4 files at r11.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @limonte, @jouni, @gatanaso, and @sudipta1411)

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. Left a few comments, but we can consider them as non-blocking.

</vaadin-demo-snippet>

<h3>Validation</h3>
<vaadin-demo-snippet id="number-field-demos-basic-valiation">
Copy link
Member

Choose a reason for hiding this comment

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

typo: validation


it('should not focus input when a button is clicked', () => {
let hasFocus = false;
input.focus = () => {
Copy link
Member

Choose a reason for hiding this comment

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

nit: would be better to use sinon.spy(input, 'focus') instead

}

:host([has-controls]) [part="value"] {
text-align: center;
Copy link
Member

Choose a reason for hiding this comment

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

note: it makes sense to include most of these styles below (excluding font-size and padding) into the core styles, as they are currently copy-pasted in both Lumo and Material

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:

Reviewed 1 of 16 files at r9, 19 of 23 files at r10, 4 of 4 files at r11.
Dismissed @sudipta1411, and @sudipta1411 from 4 discussions.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @limonte and @sudipta1411)

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.

Dismissed @limonte from a discussion.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @abdonrd and @sudipta1411)

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.

Dismissed @abdonrd from a discussion.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @sudipta1411)

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.

Dismissed @sudipta1411 from a discussion.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @gatanaso)

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.

Dismissed @manolo from a discussion.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@web-padawan web-padawan merged commit 9bed48b into master Jan 16, 2019
@web-padawan web-padawan removed the needs design Design is needed label Jan 16, 2019
@web-padawan web-padawan deleted the feature/vaadin-number-field branch February 13, 2020 20:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet