-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Set a minimum value for fontSize setting #3042
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
Set a minimum value for fontSize setting #3042
Conversation
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.
Thanks for the PR, and nice catch. Just pulled this PR down to test and it seems to be working, although I have an observation! 😄
In the current state, I can have 1px saved in settings (and visible under preferences), but the UI renders at 8px. While this looks odd, and IMO is not a deal-breaker for this use case, I think it might be worth a short investigation.
I wonder if we can use a slightly different approach where the font size is not persisted in settings unless it is an acceptable value? This might be a little involved as it seems the min/max attributes only apply when using arrows on the input. Perhaps we could use onBlur
instead of onChange
? 🤔 In any case I don't think there's a clear-cut solution for it.
I'm not super happy about it being a static value though a new const could be declared in setFont
This can be placed in constants.js
, which already contains constants of this nature 👍🏽
insomnia/packages/insomnia-app/app/common/constants.js
Lines 140 to 151 in 26fb78a
// UI Stuff | |
export const MAX_SIDEBAR_REMS = 45; | |
export const MIN_SIDEBAR_REMS = 0.75; | |
export const COLLAPSE_SIDEBAR_REMS = 3; | |
export const SIDEBAR_SKINNY_REMS = 10; | |
export const MAX_PANE_WIDTH = 0.99; | |
export const MIN_PANE_WIDTH = 0.01; | |
export const MAX_PANE_HEIGHT = 0.99; | |
export const MIN_PANE_HEIGHT = 0.01; | |
export const DEFAULT_PANE_WIDTH = 0.5; | |
export const DEFAULT_PANE_HEIGHT = 0.5; | |
export const DEFAULT_SIDEBAR_WIDTH = 19; |
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.
Oops my bad, I hit approve although there is an item worth investigating! Once you have a chance to look at it, we can re-review 🤗
This would definitely solve part of the issue with the visual aspect. There would still be the problem that you can close the settings with an invalid value which is then silently reset and that seems like bad UX. What if we disallow closing the preference or swapping to a different tab while an invalid minimum value is set? Though that would be a bit of an undertaking to ensure it's handled properly everywhere...
I think that's a good idea and it's a big improvement. We can't use that for the editor text size but since it's in the background it doesn't matter as much and isn't nearly as jarring. Good idea!
Okay great I'll move it there. I was also thinking we could bump the font max to Edit: I'm thinking of displaying an error/alert when a value is set incorrectly for the font size. I'll work on it a bit. |
I agree this will be pretty tricky and probably not worth the effort. See how far you get with a short investigation; @nijikokun could you please provide some product input on the above comments and whether the following statement is a blocker? I encourage you to pull down this PR and run it to see how it behaves.
|
964e91b
to
e057c69
Compare
377e5df
to
864991f
Compare
Added the constants as suggested and a potential option where a Demo: const_dialog_compressed.mp4 |
Moved things to consolidate and I noticed rendering was a bit slow where it was. I'm not quite sure of the best way to force the update in the
This is pretty messy though... |
I agree with this, resetting is not great, a common pattern for these issues is when the user sets a value below the threshold it snaps to the threshold value along with a tooltip that describes the allowed minimum / maximum value. Additionally, any value lower or higher than the threshold also snaps to the threshold value as well. |
Nice investigations! I agree the ternary inside Regardless of what UX approach is used (as per Niji's response above), I think this is achievable as a general pattern for number entries using something like the following, inside const el = e.currentTarget;
if (el.type === 'number') {
value = parseInt(el.value, 10) || 0;
const min = parseInt(el.min, 10);
const max = parseInt(el.max, 10);
const lessThanMax = Number.isNaN(max) || value <= max;
const moreThanMin = Number.isNaN(min) || value >= min;
const between = lessThanMax && moreThanMin;
// If not between, exit early
// or snap to min
// or snap to max
if (!lessThanMax) {
value = max;
}
}
this.props.updateSetting(el.name, value); |
@nijikokun What would the tooltip look like to you? Right now it gets the orangish outline and then on hover over says what the actual minimum would be. I looked through some of the existing tooltips, I guess maybe show a modified tooltip that actually has the text in red or something? I'm open to ideas since it seems like the alert is understandably seen as being a bit aggressive. @develohpanda Something like this? If there's a
I like the idea of moving it in to |
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.
🔥
Right now it gets the orangish outline and then on hover over says what the actual minimum would be.
This seems fine to me.
and "snapping" it back to the appropriate value visually
What you've got in the latest commit seems OK to me to be honest. It shows the above error if out of bounds; it doesn't set the UI font to absurd values like 1px, and it resets to the min/max when reopening the modal.
If however, you wanted the UI to snap back visually, immediately, you could try apply a unique key
attribute to the element which would tell React to re-mount that element, or yes, force an update on the entire General
component. If you wanted to explore this route, here's an example of forceRefreshKey
used in wrapper.js
forceRefreshKey: number, |
After further consideration on this I don't think having it automatically snap back is a good idea. That means users never see the out of bounds notification while also being prevented from entering in values which are out of bounds which is confusing UX and I could see it leading to further issues. |
Deploy preview for insomnia-storybook ready! Built with commit 505cb00 |
Closes #3041
Small fix for #3041. This sets the minimum possible
fontSize
value that can be set via the preferences menu to 8 as specified in https://github.com/Kong/insomnia/blob/develop/packages/insomnia-app/app/ui/components/settings/general.js#L271This is what the fix looks like:
font_resizing_bug_fix_compressed.mp4
When coming to this solution I considered a couple of options that included things such as creating a new
fontMinimum
settings value that was passed through, or creating a newBaseSettings
value but decided against it due to the complexity that would add without a lot of gain. There was already precedent set for this sort ternary check, though I'm not super happy about it being a static value though a new const could be declared insetFont
. I didn't pass through any sort ofmin
variable because I was concerned about how_handleFontSizeChange
would be impacted if I changed the type ofel
fromSyntheticEvent<HTMLInputElement>
.This also doesn't fix
editorFontSize
(noted it was also a small problem in the original issue) as the way that is set is different and there is noeditor-font-size
property to set so it doesn't work any way.