-
Notifications
You must be signed in to change notification settings - Fork 1
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
mrc-4011 Add step prop to number input #45
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #45 +/- ##
==========================================
Coverage ? 100.00%
==========================================
Files ? 2
Lines ? 49
Branches ? 34
==========================================
Hits ? 49
Misses ? 0
Partials ? 0 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey emma, nice job, the stepper works as expected. However, while i don't think this is a super important point, it is good to keep in mind: the read only form will only display values with precision of 0.001. So for example if you set the step to 0.0005, it will not accurately display the value. Perhaps that is a note to include in the documentation or something like that. For our purposes however, i don't see that being particularly important!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Just some suggestions
.github/workflows/test.yml
Outdated
@@ -0,0 +1,15 @@ | |||
name: test | |||
on: [push] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
on: push:
branches:
- 'main'
pull_request:
branches:
- 'main'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call, don't need it on every push. But I don't want to limit the PR branches to 'main' as we do sometimes have PRs into non-main branches, so I won't specify branch for `pull-request'
.github/workflows/test.yml
Outdated
with: | ||
node-version: 12.x | ||
- run: npm install -g codecov | ||
- run: npm install |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- run: npm install | |
- run: npm ci |
Good point, will add to docs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand we may need to trigger a job when pull request is opened against other branches. However, you have not specified any target and I do not think you can have an empty pull_request:
without a target.
.github/workflows/test.yml
Outdated
push: | ||
branches: | ||
- 'main' | ||
pull_request: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pull_request: | |
pull_request: | |
branches: | |
types: [opened, reopened, edited] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you can, just a bit confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've put in a ~ which should hopefully make it a bit more obvious that it's an empty 'any' target
.github/workflows/test.yml
Outdated
push: | ||
branches: | ||
- 'main' | ||
pull_request: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you can, just a bit confusing.
We need to provide custom step values for MINT as an increment of 1 is not appropriate for all parameter values. This adds optional
step
prop to number inputs.Test by viewing
example/index.html
and note that under 'Advanced Options', the 'Decimal parameter' value increments by 0.1 using the spinner arrows.Some additional housekeeping that was required: