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

Navbar styling rework #25343

Merged
merged 14 commits into from
Jun 20, 2023
Merged

Navbar styling rework #25343

merged 14 commits into from
Jun 20, 2023

Conversation

silverwind
Copy link
Member

@silverwind silverwind commented Jun 18, 2023 •
edited
Loading

  • Extract navbar CSS to own file
  • Reduce height from 52px to 50px
  • Give every item a hover effect of of 36px, including the logo and on mobile
  • Consistent horizontal padding of 10px left and right
Screenshot 2023-06-18 at 13 41 16 Screenshot 2023-06-18 at 14 03 43 Screenshot 2023-06-18 at 14 03 18 Screenshot 2023-06-18 at 13 58 28

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jun 18, 2023
@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 18, 2023
@silverwind silverwind added type/enhancement An improvement of existing functionality topic/ui Change the appearance of the Gitea UI labels Jun 18, 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 Jun 19, 2023
@silverwind
Copy link
Member Author

@wxiaoguang care to review?

@wxiaoguang
Copy link
Contributor

I do not understand the layout ".menu > .navbar-left > .item", it doesn't seems a correct Fomantic UI layout.

If it wants to use Fomantic Menu, then I think it's better to follow Fomantic UI's examples. If it doesn't want to, then do not use "menu"

@silverwind
Copy link
Member Author

We can likely remove the fomantic menu, but it'll require a bit of more CSS as stuff like .active link styling relies on it.

wxiaoguang
wxiaoguang approved these changes Jun 19, 2023
View reviewed changes
Copy link
Contributor

@wxiaoguang wxiaoguang left a comment •
edited
Loading

Choose a reason for hiding this comment

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

It seems fragile to do ".menu > .navabr-left > .item" and ".menu > .navabr-right > .item"

If it needs to re-use Fomantic styles, it could be:

<nav>
   <div class="navbar-left ui menu">
      <div class=item></div>
   </div>
   <div class="navbar-right ui menu">
      <div class=item></div>
   </div>
</nav>

Then it doesn't violate Fomantic UI structures.

While I won't block it.

@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 Jun 19, 2023
@silverwind
Copy link
Member Author

IIRC, the problem that .item is not always a direct descendant of .menu has to do with the hacky mobile switch.

@silverwind silverwind marked this pull request as draft June 19, 2023 10:33
@silverwind
Copy link
Member Author

silverwind commented Jun 19, 2023 •
edited
Loading

I will have a look whether this can be cleaned up easily.

@silverwind
Copy link
Member Author

Wow this turns out too be way too tricky because of how much of a hack this mobile menu is.

@silverwind
Copy link
Member Author

silverwind commented Jun 19, 2023 •
edited
Loading

It seems fragile to do ".menu > .navabr-left > .item" and ".menu > .navabr-right > .item"

If it needs to re-use Fomantic styles, it could be:

<nav>
   <div class="navbar-left ui menu">
      <div class=item></div>
   </div>
   <div class="navbar-right ui menu">
      <div class=item></div>
   </div>
</nav>

Then it doesn't violate Fomantic UI structures.

While I won't block it.

Implemented this now, it took some effort with making mobile menu flex-wrap and the first two items 50% width, but it seems to work fine now.

Also, had to use one z-index for the menu hover effect to not overlay the notifications counter:

Screenshot 2023-06-20 at 00 15 23

@silverwind silverwind marked this pull request as ready for review June 19, 2023 22:40
@silverwind silverwind marked this pull request as draft June 20, 2023 05:34
@silverwind silverwind marked this pull request as ready for review June 20, 2023 19:03
@silverwind silverwind added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Jun 20, 2023
@silverwind silverwind enabled auto-merge (squash) June 20, 2023 19:04
@silverwind silverwind disabled auto-merge June 20, 2023 19:27
@silverwind
Copy link
Member Author

There's some regression in the dropdown menus, fixing...

@silverwind
Copy link
Member Author

All fixed. These nested .item were the problem.

@silverwind silverwind enabled auto-merge (squash) June 20, 2023 19:50
@silverwind silverwind merged commit e50c3e8 into go-gitea:main Jun 20, 2023
@GiteaBot GiteaBot added this to the 1.21.0 milestone Jun 20, 2023
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Jun 20, 2023
@silverwind silverwind deleted the navrework branch June 20, 2023 20:48
@silverwind silverwind mentioned this pull request Jun 20, 2023
Merged
zjjhot added a commit to zjjhot/gitea that referenced this pull request Jun 21, 2023
@zjjhot
Merge remote-tracking branch 'giteaofficial/main'
3e8a29d 
* giteaofficial/main:
  Refactor path & config system (go-gitea#25330)
  Add actor and status dropdowns to run list (go-gitea#25118)
  Use the new download domain replace the old (go-gitea#25405)
  Avoid polluting config file when "save" (go-gitea#25395)
  Fix dropdown icon layout on diff page (go-gitea#25397)
  Support configuration variables on Gitea Actions (go-gitea#24724)
  Substitute variables in path names of template repos too (go-gitea#25294)
  Navbar styling rework (go-gitea#25343)
  Fix blank dir message when uploading files from web editor (go-gitea#25391)
  Add git-lfs support to devcontainer (go-gitea#25385)
  Use qwtel.sqlite-viewer instead of alexcvzz.vscode-sqlite (go-gitea#25386)
  Use Actions git context instead of dynamically created buildkit one (go-gitea#25381)
  rename tributeValues to mentionValues (go-gitea#25375)
  Fix LDAP sync when Username Attribute is empty (go-gitea#25278)
  Fetch all git data for embedding correct version in docker image (go-gitea#25361)
  Fix sidebar label dropdown divider (go-gitea#25359)
  Fix issue filters on mobile view (go-gitea#25368)
  Refactor: TotalTimest return seconds (go-gitea#25370)
silverwind added a commit to silverwind/gitea that referenced this pull request Jun 21, 2023
@silverwind
Merge remote-tracking branch 'origin/main' into uitweaks2
e9a5783 
* origin/main: (47 commits)
  Move some regexp out of functions (go-gitea#25430)
  Show outdated comments in files changed tab (go-gitea#24936)
  Remove "CHARSET" config option for MySQL, always use "utf8mb4" (go-gitea#25413)
  Fine tune project board label colors and modal content background (go-gitea#25419)
  Fix missing commit message body when the message has leading newlines (go-gitea#25418)
  add python/poetry to devcontainer (go-gitea#25407)
  Refactor path & config system (go-gitea#25330)
  Add actor and status dropdowns to run list (go-gitea#25118)
  Use the new download domain replace the old (go-gitea#25405)
  Avoid polluting config file when "save" (go-gitea#25395)
  Fix dropdown icon layout on diff page (go-gitea#25397)
  Support configuration variables on Gitea Actions (go-gitea#24724)
  Substitute variables in path names of template repos too (go-gitea#25294)
  Navbar styling rework (go-gitea#25343)
  Fix blank dir message when uploading files from web editor (go-gitea#25391)
  Add git-lfs support to devcontainer (go-gitea#25385)
  Use qwtel.sqlite-viewer instead of alexcvzz.vscode-sqlite (go-gitea#25386)
  Use Actions git context instead of dynamically created buildkit one (go-gitea#25381)
  rename tributeValues to mentionValues (go-gitea#25375)
  Fix LDAP sync when Username Attribute is empty (go-gitea#25278)
  ...
@wxiaoguang wxiaoguang mentioned this pull request Jun 22, 2023
Closed
lunny pushed a commit that referenced this pull request Jun 24, 2023
@silverwind
Navbar fixes (#25402)
be47015 
Fixes: #25444

Followup for some regressions from
#25343. Before and after:

<img width="219" alt="Screenshot 2023-06-21 at 00 25 20"
src="https://github.com/go-gitea/gitea/assets/115237/08fe8e01-0a16-4cdf-ad4d-0a9048408e9e">
<img width="220" alt="Screenshot 2023-06-21 at 00 25 32"
src="https://github.com/go-gitea/gitea/assets/115237/be25ae69-6ed0-4af5-8eeb-d7b210e7c124">

Fixes mobile button background and margins:

<img width="836" alt="Screenshot 2023-06-21 at 00 39 58"
src="https://github.com/go-gitea/gitea/assets/115237/d76ac1e9-747f-477c-9a42-b73e129b72ee">
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Sep 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@lunny lunny lunny approved these changes

@wxiaoguang wxiaoguang wxiaoguang approved these changes

Assignees
No one assigned
Labels
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.21.0
Development

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