Skip to content

Character limit label#1646

Merged
AaronVanGeffen merged 12 commits intoOpenLoco:masterfrom
BartoszKedziorek:characterLimit
Sep 27, 2022
Merged

Character limit label#1646
AaronVanGeffen merged 12 commits intoOpenLoco:masterfrom
BartoszKedziorek:characterLimit

Conversation

@BartoszKedziorek
Copy link
Copy Markdown
Contributor

@BartoszKedziorek BartoszKedziorek commented Sep 20, 2022

This PR refers to this issue: #1608

New label and counter, whichs allow user to see the characters limit and how many space is left.
Changes:

  • new strings in language files
  • new stringID
  • new field and method in InputSession class
  • changes in OpenLoco::Ui::Windows::TextInput::draw

Example:
picture1
picture2

I was thinking about putting it between bottom-left corner of textbox and "ok" button, but in my opinion it looks good as it is.

@BartoszKedziorek
- new method and field in InputSession class for calculating charact…
5b2b0d9 
…ers left

 - new StringID named characters_left_label

 - using buffer_2039 in TextInput::draw to display new counter
@BartoszKedziorek
- changing new string_id name
689ec1d 
 - making new label display position dependent on numer of digits in buffer_2039

 - adding new string in language files
duncanspumpkin
duncanspumpkin reviewed Sep 20, 2022
View reviewed changes
duncanspumpkin
duncanspumpkin reviewed Sep 20, 2022
View reviewed changes
duncanspumpkin
duncanspumpkin reviewed Sep 20, 2022
View reviewed changes
duncanspumpkin
duncanspumpkin reviewed Sep 20, 2022
View reviewed changes
@BartoszKedziorek
- modifications to string 2263 in language files
0ef485b 
 - changes to the code that displays the characters limit
@BartoszKedziorek
clang-format
ddf0843
@duncanspumpkin
Copy link
Copy Markdown
Contributor

duncanspumpkin commented Sep 21, 2022

Looks good. I've not tested it but can't see why it wouldn't work.

LeftofZen
LeftofZen reviewed Sep 21, 2022
View reviewed changes
LeftofZen
LeftofZen reviewed Sep 21, 2022
View reviewed changes
@LeftofZen LeftofZen added changelog Requires a changelog entry enhancement-minor Changing an existing feature slightly labels Sep 21, 2022
@LeftofZen
Copy link
Copy Markdown
Contributor

LeftofZen commented Sep 21, 2022

this should get a changelog

LeftofZen
LeftofZen previously requested changes Sep 21, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@LeftofZen LeftofZen left a comment

Choose a reason for hiding this comment

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

  • remove unused method, or use it
  • use constant instead of hardcoded 199
  • add changelog

@LeftofZen
Copy link
Copy Markdown
Contributor

LeftofZen commented Sep 21, 2022 •
edited
Loading

Personally I'd lay out the gui like this, having the char count above the textbox is kind of strange for a UI layout

(and perhaps move the ok button since it now looks weird)

You could also try to be fancy and render the count as an overlay to the textbox on the right side, but that'd require more work and probably isn't worth it

edit: better idea from aaron below

@AaronVanGeffen
Copy link
Copy Markdown
Member

AaronVanGeffen commented Sep 21, 2022 •
edited
Loading

I'd show the character count left of the 'OK' button, below the input box. Leave the input box as-is, full-width.

You could use the right-align text drawing function to this end as well.

@LeftofZen
Copy link
Copy Markdown
Contributor

LeftofZen commented Sep 21, 2022

A quick google search shows most UI's put the count bottom right, so that's also an idea
https://www.google.com/search?q=text+box+with+character+limit&tbm=isch

@LeftofZen
Copy link
Copy Markdown
Contributor

LeftofZen commented Sep 21, 2022

I'd show the character count left of the 'OK' button, below the input box. Leave the input box as-is, full-width.

You could use the right-align text drawing function to this end as well.

I like that actually, looks nice
image

@AaronVanGeffen
Copy link
Copy Markdown
Member

AaronVanGeffen commented Sep 21, 2022

This begs the question if character limits are actually set correctly, as company names are actually limited to 32 characters.

@duncanspumpkin
Copy link
Copy Markdown
Contributor

duncanspumpkin commented Sep 21, 2022

This begs the question if character limits are actually set correctly, as company names are actually limited to 32 characters.

31 characters as you need the null

@BartoszKedziorek BartoszKedziorek mentioned this pull request Sep 23, 2022
Merged
AaronVanGeffen
AaronVanGeffen reviewed Sep 27, 2022
View reviewed changes
@AaronVanGeffen
Copy link
Copy Markdown
Member

AaronVanGeffen commented Sep 27, 2022 •
edited
Loading

I've resolved the conflicts this PR had as a result of merging #1651, and moved the label to look as proposed. Works nicely!

image

@AaronVanGeffen AaronVanGeffen enabled auto-merge (squash) September 27, 2022 19:36
@AaronVanGeffen AaronVanGeffen merged commit a2e8dbf into OpenLoco:master Sep 27, 2022
@BartoszKedziorek
Copy link
Copy Markdown
Contributor Author

BartoszKedziorek commented Sep 27, 2022

@AaronVanGeffen @duncanspumpkin @LeftofZen. Thank you for all your reviews and help guys. Appreciate it :)

@LeftofZen LeftofZen added this to the v22.09+ milestone Sep 28, 2022
@LeftofZen
Copy link
Copy Markdown
Contributor

LeftofZen commented Sep 28, 2022

@BartoszKedziorek my pleasure, thanks for a great feature and nice PR. Just a quick note for future PRs, after it's merged you can update the milestone to the next release as I did above ^

@AaronVanGeffen
Copy link
Copy Markdown
Member

AaronVanGeffen commented Sep 28, 2022

after it's merged you can update the milestone to the next release as I did above

I believe only maintainers can do this, fwiw.

@LeftofZen
Copy link
Copy Markdown
Contributor

LeftofZen commented Sep 28, 2022

I believe only maintainers can do this, fwiw.

Oh I didn't realise, sorry! I'm happy to add the versions anyways

@LeeSpork LeeSpork mentioned this pull request Nov 9, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@duncanspumpkin duncanspumpkin duncanspumpkin left review comments

@LeftofZen LeftofZen LeftofZen left review comments

@AaronVanGeffen AaronVanGeffen AaronVanGeffen approved these changes

Assignees

No one assigned

Labels

changelog Requires a changelog entry enhancement-minor Changing an existing feature slightly

Projects

None yet

Milestone

v22.10

Development

Successfully merging this pull request may close these issues.

4 participants

@BartoszKedziorek @duncanspumpkin @LeftofZen @AaronVanGeffen