-
Notifications
You must be signed in to change notification settings - Fork 6
Add Number component #21
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
base: main
Are you sure you want to change the base?
Conversation
src/components/Number.ts
Outdated
|
|
||
| // Constrain value to min and max if provided | ||
| if (this.max != null) { | ||
| value = Math.min(value, parseFloat(this.max)); |
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.
💡 Suggestion (blocking): With other components not requiring the value to be valid before persisting, we should consider removing these checks. In their current form, the checks also don't truly reflect the valid state, as per HTML input element, as the max (to be valid) can also be dependent on min and step when they're present, for example:
| Min | Max | Step | Valid Max |
|---|---|---|---|
undefined |
undefined |
10 |
10 |
0 |
3 |
10 |
9 |
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 have re-ordered so the step is applied first, this means the value will always be within the valid min/max range.
On the whole I don't think we need to stress too much about people setting incompatible min/max/step values.
This is already a bit stricter than the standard number input. It allows you to enter what ever you like and only applies the min/max/step if you use the arrows. Here the limits are always applied.
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.
Based on the current algorithm, there are a couple of potential issues:
- The
stepcalculation currently indexes from0, but would need to index fromminif defined. - The rounding within the
stepcalculation would not reliably work in the case wherestepis a fraction.
With this in mind, I would propose the following change:
- The introduction of a new
clampattribute of typebooleanwith a default offalse. - When
true, the value is clamped between theminandmaxprior to saving. - When
false, the value is unchanged.
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.
- The
stepcalculation currently indexes from0, but would need to index fromminif defined.
I have reworked to index from min if provided, else 0. This is inline with what input type number does.
- The rounding within the
stepcalculation would not reliably work in the case wherestepis a fraction.
There are some cases where you see a floating point issue and get something like 0.001000000000001, I guess that is what you mean? Certainty not ideal, but tricky to fix.
With this in mind, I would propose the following change:
clamp attribute added works as you suggested. Note that the step is always applied if provided and not affected by the new attribute.
src/components/Number.ts
Outdated
| } | ||
|
|
||
| .number-units { | ||
| width: 6ch; |
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.
💡 Suggestion: To improve scalability, I would recommend we remove width here.
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.
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 a named slot (e.g. suffix) would be better here to provide more flexibility? This would then allow the Maker to specify the width should they have differing content next to their number elements.
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.
That works, I have done that, usage example:
<sdpi-item label="Number">
<sdpi-number setting="numberSetting" min="1" max="5" step="2" default="3" clamp>
<span slot="suffix" class="number-units"> Px</span>
</sdpi-number>
</sdpi-item>
With:
<style>
.number-units {
width: 6ch;
padding-left: var(--spacer);
display: flex;
align-items: center;
}
</style>
|
@GeekyEggo Bump on this, can you take another look Please. |
|
@GeekyEggo Bump on this again, I have fixed your review issues. |



This adds a number component with optional min/max/step and units.
closes #18