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

Added: SpinnerBox percentage symbol. #3777

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

Luc-Mcgrady
Copy link
Contributor

Adds the trailing % as referenced in #3679

Displays a percentage directly following the percentage values in the deck config.

percentage

Supports i18n:
deck-config-percent-input = %{ $pct }
image
deck-config-percent-input = { $pct } %
image

Known issue: The percentage sign flies off the right hand side of the box if the user fills the input to the extent where it gets horizontal scroll.

Sorry if doing it using invisible characters is a little hacky, I couldn't think of a better way.

@YukiNagat0
Copy link
Contributor

I will suggest the most obvious thing, but maybe just change the Desired retention and Historical retention strings to Desired retention (%) and Historical retention (%)? In this case, it will not be necessary to invent a bicycle for the input field. And overall, it will be more customary/usual UI decision, because it is very rare to see the input field, that have UI elements (percent sign in this case) in it.

@dae
Copy link
Member

dae commented Jan 30, 2025

Adjusting the label would have been sufficient, but Luc's done the work at this point, so it's a harder call.

On the one hand, it's the most readable for users, and we do have precedent (e.g. user interface size in preferences), as Qt provides such a widget already. On the other hand, this introduces more things that can go wrong. For example, the layout is different on iOS, where I found I had to change the 1px top to 3.5px instead to get it to align with rest of the text.

I don't have strong feelings. Anyone else want to weigh in?

@dae
Copy link
Member

dae commented Feb 3, 2025

Ok Luc, without other feedback, I will approve this. Before I can merge, please make it use a 3.5px top on iOS.

@Luc-Mcgrady
Copy link
Contributor Author

That should hopefully do it. I don't own an apple device so can't test it. I am hopeful as my solution is also used here:

@supports (-webkit-touch-callout: none) {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants