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

remove state from stepper input and simplify #1264

Merged
merged 2 commits into from
Jul 29, 2022

Conversation

davegarthsimpson
Copy link
Contributor

@davegarthsimpson davegarthsimpson commented Jul 28, 2022

Motivation

The settings-step-input.tsx component 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

  • PR addresses a single concern.
  • The PR has no duplicates (please search among the Pull Requests before creating one)
  • PR title and description are properly filled.
  • Docs have been added / updated (for bug fixes / features)

@davegarthsimpson davegarthsimpson requested a review from fstasi July 28, 2022 15:46
@davegarthsimpson davegarthsimpson added topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project labels Jul 28, 2022
@davegarthsimpson davegarthsimpson requested a review from per1234 July 28, 2022 15:58
@@ -86,34 +52,14 @@ const SettingsStepInput: React.FC<SettingsStepInputProps> = (
const number = Number(eventValue);

if (!isNaN(number) && number !== value) {
let newValue;
Copy link
Contributor

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?
}

Copy link
Contributor Author

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.

Copy link
Contributor

@kittaakos kittaakos left a 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!

@AlbyIanna
Copy link
Contributor

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

@francescospissu
Copy link
Contributor

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.

@davegarthsimpson
Copy link
Contributor Author

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.

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.

@davegarthsimpson davegarthsimpson merged commit 55927ac into main Jul 29, 2022
@davegarthsimpson davegarthsimpson deleted the refactor-stepper-react-component branch July 29, 2022 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants