Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Let the user edit the font size settings with the keyboard #1547

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

Merged
AlbyIanna merged 5 commits into main from fix-stepper-behavior
Oct 21, 2022

Conversation

Copy link
Contributor

@AlbyIanna AlbyIanna commented Oct 7, 2022

Motivation

It's difficult for the user to edit the font size in the preferences using the keyboard.

On the current state, when a user tries to edit the value of the font size with the keyboard, the value would update unexpectedly.

For example:

  • the font size is 72
  • focus the input field
  • press BACKSPACE
  • expected: the font size is 7; result: the font size is 8 😢
  • now press 1
  • expected: the font size is 81; result: the font size is 72 😢

The reason is we validate the user input as they type, and if the input is invalid, we change the value to a valid one.

Change description

With this change, I want to validate the user input as they finished typing instead of as they type. In order to achieve that, I used the onBlur callback of the input field, so that the user can type whatever they want in the input field (only numbers, to be precise), and as soon as their focus change from the input field to another element (e.g.: they click on any other piece of UI or press ESCAPE) we validate the input they just typed in and, if necessary, change it to a valid value.

Before the change (video):
Screen.Recording.2022年10月07日.at.18.26.17.mov
After the change (video):
Screen.Recording.2022年10月07日.at.18.26.43.mov

Other information

Fixes #1542

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)

@AlbyIanna AlbyIanna changed the title (削除) let the user edit the stepper input with keyboard (削除ここまで) (追記) Let the user edit the stepper input with keyboard (追記ここまで) Oct 7, 2022
@AlbyIanna AlbyIanna changed the title (削除) Let the user edit the stepper input with keyboard (削除ここまで) (追記) Let the user edit the font size settings with the keyboard (追記ここまで) Oct 7, 2022
@davegarthsimpson davegarthsimpson added type: enhancement Proposed improvement topic: code Related to content of the project itself labels Oct 11, 2022
Copy link
Contributor

@davegarthsimpson davegarthsimpson left a comment
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unfortunately there are a few issues here:

  1. This breaks the arrow buttons functionality in general due to the wrong value being referenced in the onStep function
  2. I believe it's makes things more predictable / cleaner to cast the value to string only in the JSX and to number only in the onChange handler (ultimately, only where really necessary), then record "empty string" as an exception
  3. It results in a buggy feeling UX if the user clicks an arrow when the input value is '0' or '-' (the blur restores the initial value and then "steps it"), in the below changes I've made sure to disable the steppers when an invalid value is in the input
  4. the value is passed as prop from the parent, when it changes it should reset the internal state, we should use the concept of an initialValue together with a key attribute in the stepper JSX to achieve this, the internal state does not need to be updated when setSettingsStateValue is invoked as doing should trigger a reset anyway

I pushed some changes to a different branch here which resolve the above:
ce3b47b
There is also a small styling change as I noticed the position of the arrow buttons container was wrong in the font size stepper.

Copy link
Contributor

It works well for me. The only small problem I encountered occurs when an invalid value is entered in the input field when the current value is the minimum (font size 8), in this case the input value is not automatically updated as entering an invalid value while the current value is greater than the minimum.

See the video:

sketch_oct19a._.Arduino.IDE.2.0.1.2022年10月19日.15-22-16.mp4

Copy link
Contributor Author

It works well for me. The only small problem I encountered occurs when an invalid value is entered in the input field when the current value is the minimum (font size 8), in this case the input value is not automatically updated as entering an invalid value while the current value is greater than the minimum.

Nice catch! I just pushed a fix 👍

Copy link
Contributor

kittaakos commented Oct 20, 2022
edited
Loading

I did not check the code changes thoroughly, but the behavior only. If I set the font size to 73 which exceeds 72 and hit enter, the new value will be 72. If I set the value to 0, nothing changes. I expected symmetrical behavior; setting the value to 0 will set the font size to min; 8.

AlbyIanna reacted with thumbs up emoji

Copy link
Contributor

@davegarthsimpson davegarthsimpson left a comment
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

 const setValidValue = (value: number): void => {
 const clampedValue = clamp(value, minValue, maxValue);
 setValueState({ currentValue: clampedValue, isEmptyString: false });
 setSettingsStateValue(clampedValue);
 };

this doesn't feel right, if the value has changed setSettingsStateValue resets the whole component as well as the internal state (because it changes the jsx key), thus setting the internal state shouldn't be necessary, if we're having to do this things are likely muddled and we probably need to rethink the lifecycle

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.

It works for me. Thank you!

.settings-step-input-button:hover {
background: rgba(128, 128, 128, 0.8);
}
}
Copy link
Contributor

@kittaakos kittaakos Oct 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No newline at the end of file

Copy link
Contributor Author

this doesn't feel right, if the value has changed setSettingsStateValue resets the whole component as well as the internal state (because it changes the jsx key), thus setting the internal state shouldn't be necessary, if we're having to do this things are likely muddled and we probably need to rethink the lifecycle

Ok, I'll look into it better later 👍

Copy link
Contributor Author

@francescospissu would you please give it another try? @davegarthsimpson just pushed a fix.

francescospissu reacted with thumbs up emoji

Copy link
Contributor

@francescospissu francescospissu left a comment
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now it works perfectly for me. Thanks @AlbyIanna 🙂

@AlbyIanna AlbyIanna merged commit 32d904c into main Oct 21, 2022
@AlbyIanna AlbyIanna deleted the fix-stepper-behavior branch October 21, 2022 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davegarthsimpson davegarthsimpson davegarthsimpson approved these changes

@francescospissu francescospissu francescospissu approved these changes

@per1234 per1234 Awaiting requested review from per1234

+1 more reviewer

@kittaakos kittaakos kittaakos approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
topic: code Related to content of the project itself type: enhancement Proposed improvement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

It‘s difficult to change the font size

AltStyle によって変換されたページ (->オリジナル) /