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.

Sign up for GitHub

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

Jump to bottom

Change --font-weight-bold to --font-weight-semibold and 600 value, introduce new font weight variables #24827

Merged
merged 8 commits into from
May 21, 2023
Merged

Change --font-weight-bold to --font-weight-semibold and 600 value, introduce new font weight variables #24827

merged 8 commits into from
May 21, 2023

Conversation

silverwind
Copy link
Member

@silverwind silverwind commented May 20, 2023 •
edited
Loading

There was some recent discussion about this in Discord ui-design channel and the conclusion was that #24305 should have fixed their OS font installation to have semibold weights.

I have now tested this 601 weight on a Windows 10 machine on Firefox myself, and I immediately noticed that bold was excessivly bold and rendering as 700 because browsers are biased towards bolder fonts. So revert this back to the previous value.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label May 20, 2023
@pull-request-size pull-request-size bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label May 20, 2023
@silverwind silverwind added type/enhancement An improvement of existing functionality topic/ui Change the appearance of the Gitea UI labels May 20, 2023
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels May 21, 2023
@wxiaoguang
Copy link
Contributor

wxiaoguang commented May 21, 2023 •
edited
Loading

Comment could be left there (and be improved), then some Linux users without correct font environment could know the details, and it could save contributors time if similar issue comes.

And I think it needs to be backported?

@silverwind
Copy link
Member Author

Hmm, I don't feel like commenting this, 600 is a pretty standard weight. Backport will be done, yes.

@silverwind silverwind added the outdated/backport/v1.19 This PR should be backported to Gitea 1.19 label May 21, 2023
@wxiaoguang
Copy link
Contributor

We have spent a lot of time on this problem.

Maybe in the future some other users would also report similar problems, then some new maintainers without this knowledge would waste time on this problem again.

@wxiaoguang wxiaoguang mentioned this pull request May 21, 2023
Closed
@wxiaoguang
Copy link
Contributor

Well, a new issue ... Bold text no longer displayed as bold #24835

@silverwind
Copy link
Member Author

silverwind commented May 21, 2023 •
edited
Loading

We have spent a lot of time on this problem.

Maybe in the future some other users would also report similar problems, then some new maintainers without this knowledge would waste time on this problem again.

What should we write into that comment? 601 was a "odd" value that caused issues and in some cases renders as 700 which we do not want from a design perspective. Multiples of 100 seem to generally work best.

For clarity, I would like to rename the variable. As per MDN:

400 | Normal
500 | Medium
600 | Semi Bold
700 | Bold

So we could make:

--font-weight-normal
--font-weight-medium
--font-weight-semibold
--font-weight-bold

That will satisfy all future use cases. I would not comment any of these, they are standard values.

@pull-request-size pull-request-size bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels May 21, 2023
@wxiaoguang
Copy link
Contributor

I think the "situation" could be documented:

// "bold" can't be used because it's too bold for most cases
// some fonts (eg: Segoe UI on Linux) do not provide "bolding" for weight 600, end users should fix their font library

The key information:

  1. Why not use "bold", it has been asked not just one time, I also asked similar question before, the latest question see Bold text no longer displayed as bold #24835 (comment)
  2. If any user doesn't see "bold" font, that's their system's problem, not Gitea's problem.

It would save a lot of time for future developers.

wxiaoguang
wxiaoguang reviewed May 21, 2023
View reviewed changes
web_src/css/base.css Outdated Show resolved Hide resolved
wxiaoguang
wxiaoguang approved these changes May 21, 2023
View reviewed changes
Copy link
Contributor

@wxiaoguang wxiaoguang left a comment

Choose a reason for hiding this comment

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

If you wouldn't like to write comment or remove unused variable, I could do it in next PR, I won't block for nits.

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels May 21, 2023
@silverwind
Copy link
Member Author

Will do a bit more refactor to use variables everywhere.

@silverwind silverwind marked this pull request as draft May 21, 2023 19:55
@pull-request-size pull-request-size bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 21, 2023
@silverwind
Copy link
Member Author

silverwind commented May 21, 2023 •
edited
Loading

Refactor done, now all CSS uses these variables, enforced by a linter rule, and there are tailwind-matching helper classes as well for them.

@silverwind silverwind changed the title Change --font-weight-bold to 600 Change --font-weight-bold to 600 and introduce new font weight variables May 21, 2023
@silverwind silverwind changed the title Change --font-weight-bold to 600 and introduce new font weight variables Change --font-weight-bold to --font-weight-semibold and introduce new font weight variables May 21, 2023
@silverwind
Copy link
Member Author

silverwind commented May 21, 2023 •
edited
Loading

Oh, and there is one more change. Issue title went from 300 to 400, which matches GH:

Before:
Screenshot 2023-05-21 at 22 07 10

After:
Screenshot 2023-05-21 at 22 07 21

I think we should always use weights between 400 and 600, but I have kept the 300 weights for asian fonts for now as I can not really judge those.

@silverwind silverwind changed the title Change --font-weight-bold to --font-weight-semibold and introduce new font weight variables Change --font-weight-bold to --font-weight-semibold and 600 value, introduce new font weight variables May 21, 2023
@silverwind
Merge branch 'main' into font-600
77c4599 
* main:
  Rewrite logger system (go-gitea#24726)
  Support Copy Link for video attachments (go-gitea#24833)
  Fix video width overflow in markdown, and other changes to match img (go-gitea#24834)
  Improve accessibility when (re-)viewing files (go-gitea#24817)
@silverwind silverwind added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label May 21, 2023
@silverwind silverwind enabled auto-merge (squash) May 21, 2023 23:09
@silverwind silverwind added the backport/manual No power to the bots! Create your backport yourself! label May 21, 2023
@silverwind silverwind mentioned this pull request May 21, 2023
Merged
@silverwind silverwind merged commit 19993d8 into go-gitea:main May 21, 2023
@GiteaBot GiteaBot added this to the 1.20.0 milestone May 21, 2023
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label May 21, 2023
zjjhot added a commit to zjjhot/gitea that referenced this pull request May 22, 2023
@zjjhot
Merge remote-tracking branch 'giteaofficial/main'
6791aa3 
* giteaofficial/main: (27 commits)
  Fix regression: access log template, gitea manager cli command (go-gitea#24838)
  Merge message template support for rebase without merge commit (go-gitea#22669)
  [skip ci] Updated licenses and gitignores
  Support wildcard in email domain allow/block list (go-gitea#24831)
  Change `--font-weight-bold` to `--font-weight-semibold` and 600 value, introduce new font weight variables (go-gitea#24827)
  Rewrite logger system (go-gitea#24726)
  Support Copy Link for video attachments (go-gitea#24833)
  Fix video width overflow in markdown, and other changes to match img (go-gitea#24834)
  Improve accessibility when (re-)viewing files (go-gitea#24817)
  Refactor rename user and rename organization (go-gitea#24052)
  Use `CommentList` instead of `[]*Comment` (go-gitea#24828)
  Fix topics deleted via API not being deleted in org page (go-gitea#24825)
  Return `404` in the API if the requested webhooks were not found (go-gitea#24823)
  Decouple the different contexts from each other (go-gitea#24786)
  [skip ci] Updated translations via Crowdin
  Add RTL rendering support to Markdown (go-gitea#24816)
  [skip ci] Updated translations via Crowdin
  Update JS dependencies (go-gitea#24815)
  Fix duplicate tooltip hiding (go-gitea#24814)
  Mute repo names in dashboard repo list (go-gitea#24811)
  ...
lunny pushed a commit that referenced this pull request May 22, 2023
@silverwind
Set --font-weight-bold to 600 (#24840)
e81d38b 
Backport of #24827 to 1.19, just
the font weight reduction.
@wxiaoguang wxiaoguang mentioned this pull request May 22, 2023
Merged
zjjhot added a commit to zjjhot/gitea that referenced this pull request May 22, 2023
@zjjhot
Revert "Change --font-weight-bold to --font-weight-semibold and 6…
b05d1a0 
…00 value, introduce new font weight variables (go-gitea#24827)"

This reverts commit 19993d8.
@silverwind silverwind deleted the font-600 branch May 22, 2023 06:35
silverwind added a commit to silverwind/gitea that referenced this pull request May 22, 2023
@silverwind
Merge branch 'main' into notif3
d66db5e 
* main:
  Improvements for action detail page (go-gitea#24718)
  Add CRAN package registry (go-gitea#22331)
  Fix regression: access log template, gitea manager cli command (go-gitea#24838)
  Merge message template support for rebase without merge commit (go-gitea#22669)
  [skip ci] Updated licenses and gitignores
  Support wildcard in email domain allow/block list (go-gitea#24831)
  Change `--font-weight-bold` to `--font-weight-semibold` and 600 value, introduce new font weight variables (go-gitea#24827)
  Rewrite logger system (go-gitea#24726)
  Support Copy Link for video attachments (go-gitea#24833)
@lunny lunny mentioned this pull request May 22, 2023
Closed
lunny added a commit that referenced this pull request May 22, 2023
@lunny
Revert "Change --font-weight-bold to --font-weight-semibold and 6…
62a1310 
…00 value, introduce new font weight variables (#24827)"

This reverts commit 19993d8.
@lunny lunny mentioned this pull request May 22, 2023
Closed
silverwind added a commit that referenced this pull request May 22, 2023
@wxiaoguang @silverwind
Fix document and improve comment (#24844)
bb9e20e 
* Fix broken doc link:
https://github.com/go-gitea/gitea/actions/runs/5041309438/jobs/9040887385
* Improve comments about how font weight works:
#24827 (review)

---------

Co-authored-by: silverwind <me@silverwind.io>
Codeberg-org pushed a commit to Codeberg-org/gitea that referenced this pull request Jun 3, 2023
@silverwind @earl-warren
Set --font-weight-bold to 600 (go-gitea#24840)
a1776ee 
Backport of go-gitea#24827 to 1.19, just
the font weight reduction.

(cherry picked from commit e81d38b)
@belzecue
Copy link

belzecue commented Jul 6, 2023

There was some recent discussion about this in Discord ui-design channel and the conclusion was that #24305 should have fixed their OS font installation to have semibold weights.

This is correct. I am the submitter of #24305. On the still affected device (MX Linux), i just now downloaded a Linux version of Segoe-UI from here, installed it and restarted my browser, and this device is now displaying the bold/semibold text correctly.

@lunny lunny removed the outdated/backport/v1.19 This PR should be backported to Gitea 1.19 label Jul 26, 2023
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Aug 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@wxiaoguang wxiaoguang wxiaoguang approved these changes

@lunny lunny lunny approved these changes

Assignees
No one assigned
Labels
backport/manual No power to the bots! Create your backport yourself! lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. topic/ui Change the appearance of the Gitea UI type/enhancement An improvement of existing functionality
Projects
None yet
Milestone
1.20.0
Development

Successfully merging this pull request may close these issues.
5 participants
@silverwind @wxiaoguang @belzecue @lunny @GiteaBot