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

Introduce .secondary-nav and handle .page-content spacing universally #29982

Merged
merged 8 commits into from
Mar 22, 2024

Conversation

silverwind
Copy link
Member

@silverwind silverwind commented Mar 22, 2024

Fixes: #29981. Introduce .secondary-nav as a universal way for styling and margin adjustments inside .page-content.

If the first child of .page-content is .secondary-nav, we add margin below it, otherwise we add padding to the first child. Notable changes:

  • --color-header-wrapper is replaced with --color-secondary-nav-bg.
  • navbar class is removed.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 22, 2024
@pull-request-size pull-request-size bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Mar 22, 2024
@silverwind
Copy link
Member Author

Maybe later refactor to always have the overflow-menu and hide that box header.

@silverwind
Copy link
Member Author

Maybe it should not use :has because PaleMoon does not have it yet. I will check for alternatives.

@silverwind
Copy link
Member Author

Maybe it should not use :has because PaleMoon does not have it yet. I will check for alternatives.

done

@github-actions github-actions bot added the modifies/templates This PR modifies the template files label Mar 22, 2024
@wxiaoguang
Copy link
Contributor

wxiaoguang commented Mar 22, 2024

Is it really right? I guess it only hacky patches one page you know, but there are a lot.

I would say abusing tw- helpers to fine-tune everything is wrong. Please use some well-designed and stable framework-level design.

image

@wxiaoguang
Copy link
Contributor

And I would suggest to revert #29922 and leave enough time to have a better framework-level design.

@silverwind
Copy link
Member Author

silverwind commented Mar 22, 2024

It's quite simple. The first element inside .page-content sets margin, if it needs one. Here, either the <overflow-menu> or the login box is first element and the <overflow-menu> currently has a built-in margin, so adding to the box would make a double margin.

If you want it cleaner, I will remove the built-in margin on <overflow-menu> and instead add it via tw- helpers to all other ocurrences except this one, but then the diff will be 10+ files.

And no, I disagree to reverting #29922, it is a important step.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Mar 22, 2024

I am sure your hacky patches are not right.

For example, this time, you fixed one page:

image

But there is another page (I was not going to checkout the branch, but I have to .....)

image

PLEASE provide a stable framework-level design.

@silverwind
Copy link
Member Author

silverwind commented Mar 22, 2024

There is no general solution here. Some elements want margin, some don't. Some want different margin even.

It can not be generalized on the .page-content element.

@silverwind
Copy link
Member Author

silverwind commented Mar 22, 2024

So in essence we have two scenarios:

  • There is a secondary navbar. The second element needs the margin.
  • There is no secondary navbar. The first element needs the margin.

So maybe a general solution is possible but the selector for second element won't be pretty. I guess I would introduce .secondary-navbar class.

@silverwind
Copy link
Member Author

@wxiaoguang what to you think about the above suggestion? Is that "framework" enough for you?

@wxiaoguang
Copy link
Contributor

@wxiaoguang what to you think about the above suggestion? Is that "framework" enough for you?

If I understand correctly:

nvarbar 
(15 margin)
page-content
nvarbar 
secondary-navbar
(15 margin)
page-content

right? It looks like the same as what Fomantic UI does: attached segment. I think it would work.

@silverwind
Copy link
Member Author

Yes, like that. On repo page it currently uses 12px, but stuff like that can be overridden with tailwind and maybe we should just generally reduce from 15px to 12px.

@silverwind
Copy link
Member Author

Actually on repo the .header-wrapper fulfills the role of the secondary navbar and the 12px margin is internal to that, so we can still make it standard margin below.

@wxiaoguang
Copy link
Contributor

Yes, like that. On repo page it currently uses 12px, but stuff like that can be overridden with tailwind and maybe we should just generally reduce from 15px to 12px.

Some general and well-defined classes look good to me.

@silverwind
Copy link
Member Author

silverwind commented Mar 22, 2024

Ok I guess I will remove this header-wrapper class and replace it with secondary-navbar. There is also a CSS variable for it that I will rename.

@silverwind silverwind marked this pull request as draft March 22, 2024 12:01
@pull-request-size pull-request-size bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 22, 2024
@github-actions github-actions bot added modifies/translation modifies/go Pull requests that update Go code modifies/js labels Mar 22, 2024
@pull-request-size pull-request-size bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 22, 2024
@silverwind silverwind changed the title Fix login page padding with absent overflow-menu Introduce .secondary-nav and handle margin universally Mar 22, 2024
@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 Mar 22, 2024
@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 Mar 22, 2024
@silverwind silverwind added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Mar 22, 2024
@silverwind silverwind enabled auto-merge (squash) March 22, 2024 14:09
@silverwind silverwind removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Mar 22, 2024
@silverwind silverwind disabled auto-merge March 22, 2024 14:11
@silverwind
Copy link
Member Author

Leaving open a bit more.

silverwind and others added 2 commits March 22, 2024 15:11
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
@silverwind silverwind changed the title Introduce .secondary-nav and handle margin universally Introduce .secondary-nav and handle page-content spacing universally Mar 22, 2024
@silverwind silverwind changed the title Introduce .secondary-nav and handle page-content spacing universally Introduce .secondary-nav and handle .page-content spacing universally Mar 22, 2024
@silverwind silverwind added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Mar 22, 2024
@silverwind silverwind enabled auto-merge (squash) March 22, 2024 23:29
@silverwind silverwind merged commit 3ccda41 into go-gitea:main Mar 22, 2024
26 checks passed
@GiteaBot GiteaBot added this to the 1.23.0 milestone Mar 22, 2024
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Mar 22, 2024
@6543 6543 modified the milestones: 1.23.0, 1.22.0 Mar 24, 2024
zjjhot added a commit to zjjhot/gitea that referenced this pull request Mar 24, 2024
* giteaofficial/main: (28 commits)
  Enable a few stylelint rules (go-gitea#30038)
  Remove remaining jQuery .css code (go-gitea#30015)
  Respect DEFAULT_ORG_MEMBER_VISIBLE setting when adding creator to org (go-gitea#30013)
  Remove jQuery `.attr` from the common global functions (go-gitea#30023)
  Migrate font-size helpers to tailwind (go-gitea#30029)
  Replace all simple inline styles with tailwind (go-gitea#30032)
  Migrate font-weight helpers to tailwind (go-gitea#30027)
  Remove jQuery from the issue "go to" button (go-gitea#30028)
  Determine fuzziness of bleve indexer by keyword length (go-gitea#29706)
  Escape paths for find file correctly (go-gitea#30026)
  Remove jQuery `.attr` from the diff page (go-gitea#30021)
  Remove jQuery `.attr` from the repository settings (go-gitea#30018)
  Remove jQuery `.attr` from the image diff again (go-gitea#30022)
  Introduce `.secondary-nav` and handle `.page-content` spacing universally (go-gitea#29982)
  Remove jQuery `.attr` from the branch/tag selector (go-gitea#30010)
  Remove jQuery `.attr` from the commit graph (go-gitea#30006)
  Remove jQuery from the citation modal (except fomantic) (go-gitea#30008)
  Remove jQuery `.attr` from the project page (go-gitea#30004)
  Fix incorrect tailwind migration (go-gitea#30007)
  Enforce trailing comma in JS on multiline (go-gitea#30002)
  ...
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Jun 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/js modifies/templates This PR modifies the template files size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Login UI layout issue, the top marin of the login div missed.
5 participants