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

Remove text transformation from update command #2037

Merged
merged 1 commit into from
Dec 29, 2021

Conversation

Iksas
Copy link
Contributor

@Iksas Iksas commented Dec 29, 2021

By submitting this pull request, I confirm the following:

  • I have read and understood the contributors guide, as well as this entire template.
  • I have made only one major change in my proposed changes.
  • I have commented my proposed changes within the code.
  • I have tested my proposed changes.
  • I am willing to help maintain this change if there are issues with it later.
  • I give this submission freely and claim no ownership.
  • It is compatible with the EUPL 1.2 license
  • I have squashed any insignificant commits. (git rebase)
  • I have Signed Off all commits. (git commit --signoff)

What does this PR aim to accomplish?:

This PR removes the upper case text transformation from the update command. The resulting change can be seen below:

before

after

Only the LCARS theme is affected, because other themes don't use text transformations.

How does this PR accomplish the above?:

The text-transform property is set to none for code elements in the footer.

What documentation changes (if any) are needed to support this PR?:

none

@Iksas
Copy link
Contributor Author

Iksas commented Dec 29, 2021

An additional change is needed if the host name should remain capitalized.

On the other hand, this fix would improve the readability of host names using camel case.

@rdwebdesign
Copy link
Member

This was an intentional design choice. I chose the uppercase, because the original design used uppercase everywhere.

I don't know if this should be change only because personal taste.
If there is a technical reason to change this, I'm OK.

Do you have a reason for this? Can you please explain?

@Iksas
Copy link
Contributor Author

Iksas commented Dec 29, 2021

Host names are not case sensitive, so in my opinion, displaying it in all-caps is fine.

However, the update command does not work in all caps, which may confuse inexperienced users. This is why I believe the update command should not be capitalized.

Would it be okay to change the capitalization only in the footer? This would be a compromise between design and usability.

PS: I mean the capitalization of the code element in the footer, not the whole footer.

Signed-off-by: Iksas <Iksas@users.noreply.github.com>
@Iksas Iksas changed the title Remove text transformations from code elements Remove text transformations from update command Dec 29, 2021
@Iksas Iksas changed the title Remove text transformations from update command Remove text transformation from update command Dec 29, 2021
@rdwebdesign
Copy link
Member

rdwebdesign commented Dec 29, 2021

Would it be okay to change the capitalization only in the footer? This would be a compromise between design and usability.

PS: I mean the capitalization of the code element in the footer, not the whole footer.

I think this is the best solution (usability is always more important).


PS: I don't know if you are aware of this, but text formatted using text-transform: uppercase, is just a visual effect. When you copy text using this transform, the resulting text shows the original capitalization (like it was before transform is applied).
Ex: copy the footer and paste elsewhere.

@rdwebdesign rdwebdesign merged commit c4c6089 into pi-hole:devel Dec 29, 2021
@Iksas
Copy link
Contributor Author

Iksas commented Dec 29, 2021

Interesting, I didn't know that.

@PromoFaux PromoFaux mentioned this pull request Jan 4, 2022
@pralor-bot
Copy link

This pull request has been mentioned on Pi-hole Userspace. There might be relevant details there:

https://discourse.pi-hole.net/t/pi-hole-ftl-v5-13-web-v5-10-and-core-v5-8-released/52254/1

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