brave-core
brave-core copied to clipboard
brave
Removed 'Muli' font. Added 'Manrope' font. Extended 'Poppins' with 'Manrope' for Cyrillic&Greek chars.
Resolves https://github.com/brave/brave-browser/issues/4309
Submitter Checklist:
- [x] I confirm that no security/privacy review is needed, or that I have requested one
- [x] There is a ticket for my issue
- [x] Used Github auto-closing keywords in the PR description above
- [ ] Wrote a good PR/commit description
- [ ] Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
- [x] Added appropriate labels (
QA/YesorQA/No;release-notes/includeorrelease-notes/exclude;OS/...) to the associated issue - [ ] Checked the PR locally:
npm run test -- brave_browser_tests,npm run test -- brave_unit_tests,npm run lint,npm run gn_check,npm run tslint - [x] Ran
git rebase master(if needed)
Reviewer Checklist:
- [ ] A security review is not needed, or a link to one is included in the PR description
- [ ] New files have MPL-2.0 license header
- [ ] Adequate test coverage exists to prevent regressions
- [ ] Major classes, functions and non-trivial code blocks are well-commented
- [ ] Changes in component dependencies are properly reflected in
gn - [ ] Code follows the style guide
- [ ] Test plan is specified in PR before merging
After-merge Checklist:
- [ ] The associated issue milestone is set to the smallest version that the changes has landed on
- [ ] All relevant documentation has been updated, for instance:
- [ ] https://github.com/brave/brave-browser/wiki/Deviations-from-Chromium-(features-we-disable-or-remove)
- [ ] https://github.com/brave/brave-browser/wiki/Proxy-redirected-URLs
- [ ] https://github.com/brave/brave-browser/wiki/Fingerprinting-Protections
- [ ] https://github.com/brave/brave-browser/wiki/Brave%E2%80%99s-Use-of-Referral-Codes
- [ ] https://github.com/brave/brave-browser/wiki/Custom-Headers
- [ ] https://github.com/brave/brave-browser/wiki/Web-Compatibility-Exceptions-in-Brave
- [ ] https://github.com/brave/brave-browser/wiki/QA-Guide
- [ ] https://github.com/brave/brave-browser/wiki/P3A
Test Plan:
kindly ping @petemill @bsclifton @zenparsing - can you please take a look when you have some time? This fixes a P2 issue
I'll take a look at reviewing this, but I'll need to do a build to see the difference. I would suggest you append some more items the description:
- Which UIs that are affected by replacing Multi with Poppins, so we can check font-size and spacing
- Which different UIs in particular to look at for cyrillic improvements, if any
- Any other test scenarios that we should run by the design team
Some before and after screenshots would be helpful too.
Thanks!
Oh wow - this'll be a great one! (adding Cyrillic support). I can pull down and try after it's rebased
++ on everything @petemill mentioned 😄 Many of the UIs may look the same before/after and it's fine to skip screenshots. But for ones which have a difference (hopefully not a big spacing difference or line height difference, etc) it's good to see what that could be
Fonts.zip See in the attachment a few screenshots for the English and Cyrillic versions. In English version, Muli is replaced by Poppins, also all Cyrillic characters are displayed in Manrope In Cyrillic version, only Manrope is used
Yeah it's looking pretty good at first glance of the screenshots. We'll want to get a designer to look over them to check the font-weight and size differences especially between Muli and Poppins. I think this is a good way of doing it since it'll improve cyrillic font display even for users who are currently viewing pages in non-cyrillic languages. Only issue may be that we have to be granular when choosing a font for different character sets.
Still some TODOs:
- we removed muli.css but not the font files.
- we have not removed all references to muli. Some references are provided by
theme.fontFamily.bodyand our default theme provided by brave-ui has a value for this property asMuli. We can replace that value in brave-ui, which seems better semantics than replacingfontFamily.bodywithfontFamily.heading, although this older design-system is being deprecated soon.
In general we have way too many css definitions of font-family. It only really needs it once per UI since the css value is inherited!
@jenn-rhim @bradleyrichter can you please look at screenshots and confirm they look good? https://github.com/brave/brave-core/pull/12933#issuecomment-1107359743
It looks great overall 😎, but here is some feedback https://www.figma.com/file/tLXWGCpNoiJxDZDdpfordj/?node-id=1996%3A38663
It looks great overall 😎, but here is some feedback https://www.figma.com/file/tLXWGCpNoiJxDZDdpfordj/?node-id=1996%3A38663
Hi, could you please take a look at this build https://brave-jenkins-build-artifacts.s3.amazonaws.com/brave-browser-build-pr/issues/4309/f6c54aadbb37e73a2ab042aad512c4ee68648fa8/windows/brave-v1.41.41-win32-x64.zip
I have done the brave-ui changes here: https://github.com/brave/brave-ui/pull/607
You can use this sha for testing that in this PR: e4c049880199ac652b8f69610b8b70d1cd6d906f
I pushed an update on this branch to the brave-ui ref for https://github.com/brave/brave-ui/pull/607 so that we can get a browser build and Storybook including it. There may be some font style and weight changes required as a result of the additional changes from Muli to Poppins that are caused by changing the theme variable value.
Ignore these for now, the brave-ui change to bring in Muli has regressed as per https://github.com/brave/brave-core/pull/12933#issuecomment-1213440012
~New (Poppins) on left and current (Muli) on right~ Click to open in 100%
Welcome page
Rewards page
