-
-
Notifications
You must be signed in to change notification settings - Fork 490
remove state from stepper input and simplify #1264
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
Conversation
arduino-ide-extension/src/browser/dialogs/settings/settings-step-input.tsx
Outdated
Show resolved
Hide resolved
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 assume the code could return with the function above 👆
if (eventValue === '') { setSettingsStateValue(0); return; // <--- this? }
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.
yes! sorry I saw this after the fact, I'll add it in when we tackle the scale range question.
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 tried it and it worked. Thank you!
The code looks fine to me overall, but I noticed a little bug.
It is possible, via shortcuts (CMD/CTRL + '+', CMD/CTRL + '-'), to exceed the min/max thresholds. For example, if the interface scale is set to 200 I can press CMD + '+' to increase the scale to 220. When the threshold is exceeded, the stepper buttons might look enabled even if they should be disabled.
Screen.Recording.2022年07月29日.at.14.58.06.mov
Nice rework👍 I tested it by comparing the behavior with RC-9 and it works as before. I also found the bug reported by @AlbyIanna but it was already present before these changes.
The code looks fine to me overall, but I noticed a little bug.
It is possible, via shortcuts (CMD/CTRL + '+', CMD/CTRL + '-'), to exceed the min/max thresholds. For example, if the interface scale is set to
200I can press CMD + '+' to increase the scale to220. When the threshold is exceeded, the stepper buttons might look enabled even if they should be disabled.
Thanks for catching this, this is actually because at the app level we are not restricting scale, we are only doing it with the stepper. This brings about the UX question: do we want to have a max/min scale. Either way the stepper itself is separate to this issue.
Uh oh!
There was an error while loading. Please reload this page.
Motivation
The
settings-step-input.tsxcomponent contains unnecessarily verbose code and inefficient logic.Change description
Refactors the code, to exclude useState and useEffect hooks.
Acceptance Criteria
The stepper component should function exactly as before (prior to the changes in this PR).
Reviewer checklist