-
Notifications
You must be signed in to change notification settings - Fork 97
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
FLUID-6754: add system-ui font option to UIO #1098
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.
This PR looks good to me. "System font" is also the best name I can come up with. Let's see what @amb26 thinks. He's much better at naming.
We had a good discussion about naming today. The suggestion is to change "System Font" to "Device Font" as it is more understandable and descriptive about where that font is originating from. Another point raised is that "Default" is now ambiguous, we've landed on changing the name to "No Preference" to indicate that it isn't based on a font that is being applied by UIO. We should also change the "default" option in contrast to be called "No Preference". |
@@ -2,6 +2,7 @@ | |||
|
|||
$fonts: ( | |||
".fl-font-arial": "Arial", | |||
".fl-font-system-ui": "system-ui", |
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.
To avoid confusing ourselves in future, should we rename all these uses of "system" to "device" also?
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.
Good question. The actual generic font-family is called system-ui. So the right hand side can't be changed. We could change the name of the class to device if that's preferred. I'm fine either way, I guess we need to decide if we want the class name to reflect the font family or the name exposed via the adjuster.
@@ -1,5 +1,5 @@ | |||
{ | |||
"contrast-default": "پیش فرض", | |||
"contrast-default": "No Preference", |
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.
The translation should be in Farsi instead of English.
The same comment applies to translation of "No Preferences" in contrast_fr.json, contrast_pt_BR.json.
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 catching this. I had forgotten that those files were updated as well. Interesting thing I had missed before is that the localization preferences already used "No Preference" instead of "Default".
Steps to produce an issue using the code in this pull request:
Note that "No preference" and "device font" are translated to Farsi but not other choices. Shall all choices be translated to Farsi? This issue happens for all language translations. |
I believe in this case it is the correct behaviour. When we originally sent these off for translation they were sent the whole file, and those font names weren't translated. I believe it's because they are proper names so there isn't a translation, and/or they are more commonly known in their English name. |
@amb26 did you have more thoughts on this PR? Should we talk more about the naming changes you were asking about before? |
option element styling has limited to no browser support.
Adds the system-ui font to UIO
https://issues.fluidproject.org/browse/FLUID-6754