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

FIX: Fixed inconsistencies between texts #1569

Closed
wants to merge 3 commits into from

Conversation

zigad
Copy link
Contributor

@zigad zigad commented Aug 9, 2022

What's new

Fixed few more inconsistencies between text on Flipper. Similar fixes as on PR #1496.

Verification

Look through changed files. I have already build a local firmware and tested it on my Flipper and it works as expected.

Checklist (For Reviewer)

  • PR has description of feature/bug or link to Confluence/Jira task
  • Description contains actions to verify feature/bugfix
  • I've built this code, uploaded it to the device and verified feature/bugfix

Fixed few more inconsistencies between text on Flipper. Similar fixes as on PR flipperdevices#1496
@xMasterX
Copy link
Contributor

xMasterX commented Aug 9, 2022

Whats the reason to make every new word in same sentence starting with capital letter?

@zigad
Copy link
Contributor Author

zigad commented Aug 10, 2022

Whats the reason to make every new word in same sentence starting with capital letter?

Hi @xMasterX,

80% of existing text in flipper firmware is in "Pascal Case" (Every New Word In Capital Letter) other 20% is just a normal sentence or just random case. I have already replaced most of those other texts in previous PR but I have missed some titles and other texts and some new text was added in new commits.

If you look at current titles of example menu items in Settings, you will see all of them are in Pascal Case, but there are some exceptions with no reason. Either it's normal casing for just each sentence or Pascal Case and since most of text in Flipper uses Pascal Case, this PR replaces texts that don't follow the Pascal Case rules so we have an unified experience across whole Flipper UI.

@sodoku
Copy link

sodoku commented Aug 10, 2022

I appreciate the work. Personally I am also not a fan of capitalising everything. It can make sense for things like headlines, but looks a bit odd to me in prompts (e.g. Enter Your Current PIN: or Remember Or Write It Down!").

Rather than pascal case, I would look at title case. Even in headlines not every word should be capitalised.

@zigad
Copy link
Contributor Author

zigad commented Aug 10, 2022

I appreciate the work. Personally I am also not a fan of capitalising everything. It can make sense for things like headlines, but looks a bit odd to me in prompts (e.g. Enter Your Current PIN: or Remember Or Write It Down!").

Rather than pascal case, I would look at title case. Even in headlines not every word should be capitalised.

@sodoku Agreed, but there again, this commit just fixes the problem that 80% of text is in Pascal Case and 20% is in just random case.

If you go for example to Settings -> Bluetooth, you will see that Pascal Case is used, same is in Settings - LCD and Notifications, and most of other menu items and splash screens.

With this PR we change everything to Pascal Case since it's easier to change 20% of text rather than other way around and fix those inconsistencies, because it's annoying to see some titles in Pascal case and others just Capitalise first Letter.

I think we should agree on what kind of naming conventions we should use in the UI and then replace everything, even if we have to change every single title in there.

@skotopes
Copy link
Member

Hi, sorry for delay.
We've been discussing this PR internally, camel case in some places are not grammatically correct. Also it will cause redo of documentation screenshots.
So we decided to not to merge this PR and instead do full text review and fix everything in future releases.

@skotopes skotopes closed this Aug 17, 2022
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.

4 participants