159 number input improvements

lucasdinonolte commented 2 years ago

What this does

This addresses #159, holding shift will cause the number input to increment by step * 10 using the arrow keys
This also addresses the issue observed by Rune that is related to the value being clamped inside the number component
- you're now allowed to type a value that is out of min/max range
- the number input component just won't emit this value, so you don't get a re-render
- the number input component will display an error message

Pending changes

I think the min/max validation shouldn't happen inside the number component but rather at the parent level
However, I believe this is a bigger refactor, as we'd need to refactor the parent, so it stops event emission if there is a validation error somewhere along its children
The current behavior (with the custom validators a user can provide) is that the function will re-render, even if some input field has a validation error

Next steps

Before tackling the potential refactor, I'd like @fdoflorenzano take on the potential refactor.

I'd also like @runemadsen to check, if this new number input behavior (allowing out-of-range values but erroring) fixes the issue he mentioned.

Also @bravomartin should check if the 10x step is what he had in mind.

netlify[bot] commented 2 years ago

Deploy Preview for dsi-logo-maker ready!

Name	Link
Latest commit	41f2172583a8327d34115b76afad5ffe23e9ceaf
Latest deploy log	https://app.netlify.com/sites/dsi-logo-maker/deploys/636a38ffac4e33000875566b
Deploy Preview	https://deploy-preview-160--dsi-logo-maker.netlify.app/
Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

runemadsen commented 2 years ago

Yezzz, this solves my problem!

runemadsen commented 2 years ago

This is a good catch @fdoflorenzano and I haven't thought about that. I've managed these situations before, and it always gets super messy. Let me think about this and let's talk about it Monday.

lucasdinonolte commented 2 years ago

I guess a useEffect call could be done where it updates the internal value when the prop value changes?? but I'm not sure of the whole implications of that.

I think I'd prefer keeping the Input components purely controlled and move the validation to the parent layer, where it could get merged with any user-provided custom validation. That would probably mean moving the validation logic to either the Input.js wrapper or the Sidebar in core, do you agree or do you think there's a better place for it?

I also noticed, that if a user provided validation fails this doesn't currently stop the function from re-rendering. If moving all validation to the parent I'd like to change that behavior, so that any failing validation (no matter if built-in or user-provided) stops the function from re-rendering.

If this proposal sounds good to you, I'm more than happy to give the needed refactor a shot on this PR :blush:

runemadsen commented 2 years ago

This is sorta what I was thinking too. I'm still not sure how it would solve our number input problem, but if @fdoflorenzano agrees, I think it would be great to give it a try and see if it can be solved this way.

fdoflorenzano commented 2 years ago

I think I'd prefer keeping the Input components purely controlled and move the validation to the parent layer, where it could get merged with any user-provided custom validation.

Yeah, that also sounds like a good road to go in to me.

If moving all validation to the parent I'd like to change that behavior, so that any failing validation (no matter if built-in or user-provided) stops the function from re-rendering.

This is where I wonder if we'll get to the same problem we are having now. I guess it makes sense for the validation to work this way, but I personally never thought about it that way.

Fine by me if you take a stab at this! If it gets tough tho, I would recommend separating that task into it's own PR so we can get the shit scaling in.

lucasdinonolte commented 1 year ago

I had another look at this, I'll do the refactor to the validation logic on another PR, as it feels like a bigger refactor. I'll close this for now.

I've cherry-picked the shift-step onto #167 for isolated review, rework and potential merge :blush:

designsystemsinternational / mechanic