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

Ui correction in mobile view nav bar left aligned items. #27046

Merged
merged 13 commits into from
Sep 16, 2023
Merged

Ui correction in mobile view nav bar left aligned items. #27046

merged 13 commits into from
Sep 16, 2023

Conversation

puni9869
Copy link
Member

As title
From the long time I was looking for this UI, Now its the time to fix it.

Before
image

image

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Sep 12, 2023
@pull-request-size pull-request-size bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Sep 12, 2023
@github-actions github-actions bot added the topic/ui Change the appearance of the Gitea UI label Sep 12, 2023
@puni9869 puni9869 changed the title Ui correction in mobile view nav bar. Ui correction in mobile view nav bar left aligned items. Sep 12, 2023
@denyskon
Copy link
Member

denyskon commented Sep 12, 2023 •
edited
Loading

Ehm... I don't really see a reason to "fix" this. I think it looks better when it is centered.

@puni9869
Copy link
Member Author

According to the Ux in mobile view all the nav items are left aligned IMO.

I am open for suggestions.

@denyskon
Copy link
Member

To me, it looks better to have them centered. If they're all left-aligned, we have A LOT of empty space on the right, which doesn't look good to me. Just my opinion - I won't block if other maintainers disagree with me

@puni9869
Copy link
Member Author

@wxiaoguang any thoughts on this.

@silverwind
Copy link
Member

silverwind commented Sep 13, 2023 •
edited
Loading

I think left-aligned does look slightly better, but I think it could be done CSS only with justify-content: flex-start instead of introducing wrappers. Just need to be careful with the selector.

@puni9869
Copy link
Member Author

puni9869 commented Sep 13, 2023 •
edited
Loading

changes done.

@wxiaoguang
Copy link
Contributor

@wxiaoguang any thoughts on this.

I seldom use mobile to work, while maybe "keeping the dropdown menu on mobile" is also a choice, then there won't be alignment or extra-space problem any more.

@puni9869
Copy link
Member Author

@wxiaoguang any thoughts on this.

I seldom use mobile to work, while maybe "keeping the dropdown menu on mobile" is also a choice, then there won't be alignment or extra-space problem any more.

Agree.

@puni9869 puni9869 requested review from silverwind and lunny and removed request for silverwind September 13, 2023 13:19
@silverwind
Copy link
Member

Looks like a mess. Duplicate classes, wrappers not entirely removed and I recommend adding a justify-start helper for this, ideally without !important on it.

https://tailwindcss.com/docs/justify-content

@puni9869
Copy link
Member Author

Looks like a mess. Duplicate classes, wrappers not entirely removed and I recommend adding a justify-start helper for this, ideally without !important on it.

https://tailwindcss.com/docs/justify-content

worth of giving a try. I am giving a try if tailwind is installed in gitea.

@silverwind
Copy link
Member

I guess I will soon contribute either tailwind or unocss. Have not decided whether to !important those tailwind rules, but I guess it's likely it will have to be to ensure proper overrides of fomantic. So I would not mind if you add the helper with !important.

@wxiaoguang
Copy link
Contributor

I am sure many helpers are not necessary. For this case, why not just clean up #navbar .item ? I do not think it is right to keep overriding styles.

@pull-request-size pull-request-size bot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 16, 2023
@wxiaoguang
Copy link
Contributor

wxiaoguang commented Sep 16, 2023 •
edited
Loading

According to my test, the only thing needs to do is to remove the justify-content: center; if you want to make the items left-aligned. Feel free to revert my change if it is wrong. (maybe in the future "keeping the dropdown menu on mobile" is a better choice, then there won't be alignment or extra-space problem any more)

@puni9869
Copy link
Member Author

puni9869 commented Sep 16, 2023 •
edited
Loading

According to my test, the only thing needs to do is to remove the justify-content: center; if you want to make the items left-aligned. Feel free to revert my change if it is wrong. (maybe in the future "keeping the dropdown menu on mobile" is a better choice, then there won't be alignment or extra-space problem any more)

I was about to push this change today, but you did it. already. Thanks a ton. I will give a test quick.

@puni9869
Copy link
Member Author

And thanks for accepting this change from UX perspective.

@puni9869
Copy link
Member Author

Tested, look good to me.
🚀

@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 Sep 16, 2023
@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 Sep 16, 2023
@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Sep 16, 2023
@puni9869
Copy link
Member Author

Let's roll 🚀

@silverwind silverwind merged commit a1b2a11 into go-gitea:main Sep 16, 2023
25 checks passed
@GiteaBot GiteaBot added this to the 1.22.0 milestone Sep 16, 2023
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Sep 16, 2023
@delvh delvh modified the milestones: 1.22.0, 1.21.0 Sep 16, 2023
zjjhot added a commit to zjjhot/gitea that referenced this pull request Sep 17, 2023
@zjjhot
Merge remote-tracking branch 'giteaoffical/main'
1434f7a 
* giteaoffical/main: (23 commits)
  Search branches (go-gitea#27055)
  Fix wrong migration for email address (go-gitea#27106)
  [skip ci] Updated translations via Crowdin
  Support `.git-blame-ignore-revs` file (go-gitea#26395)
  Add `RemoteAddress` to mirrors (go-gitea#26952)
  Upgrading the actions/checkout@4 (go-gitea#27096)
  Next round of `db.DefaultContext` refactor (go-gitea#27089)
  Ui correction in mobile view nav bar left aligned items. (go-gitea#27046)
  Add missing deps to files-changed (go-gitea#27100)
  Use db.WithTx for AddTeamMember to avoid ctx abuse (go-gitea#27095)
  Drop Node.js 16 and update js dependencies (go-gitea#27094)
  Fix NPE when editing OAuth2 applications (go-gitea#27078)
  Use `print` instead of `printf` (go-gitea#27093)
  Add tests for db indexer in indexer_test.go (go-gitea#27087)
  [skip ci] Updated translations via Crowdin
  Allow empty Conan files (go-gitea#27092)
  Actions are no longer experimental, so enable them by default (go-gitea#27054)
  Update brew installation documentation since gitea moved to brew core package (go-gitea#27070)
  More refactoring of `db.DefaultContext` (go-gitea#27083)
  [skip ci] Updated translations via Crowdin
  ...
silverwind added a commit to silverwind/gitea that referenced this pull request Sep 17, 2023
@silverwind
Merge remote-tracking branch 'origin/main' into usefetch
8a6e43c 
* origin/main: (53 commits)
  Search branches (go-gitea#27055)
  Fix wrong migration for email address (go-gitea#27106)
  [skip ci] Updated translations via Crowdin
  Support `.git-blame-ignore-revs` file (go-gitea#26395)
  Add `RemoteAddress` to mirrors (go-gitea#26952)
  Upgrading the actions/checkout@4 (go-gitea#27096)
  Next round of `db.DefaultContext` refactor (go-gitea#27089)
  Ui correction in mobile view nav bar left aligned items. (go-gitea#27046)
  Add missing deps to files-changed (go-gitea#27100)
  Use db.WithTx for AddTeamMember to avoid ctx abuse (go-gitea#27095)
  Drop Node.js 16 and update js dependencies (go-gitea#27094)
  Fix NPE when editing OAuth2 applications (go-gitea#27078)
  Use `print` instead of `printf` (go-gitea#27093)
  Add tests for db indexer in indexer_test.go (go-gitea#27087)
  [skip ci] Updated translations via Crowdin
  Allow empty Conan files (go-gitea#27092)
  Actions are no longer experimental, so enable them by default (go-gitea#27054)
  Update brew installation documentation since gitea moved to brew core package (go-gitea#27070)
  More refactoring of `db.DefaultContext` (go-gitea#27083)
  [skip ci] Updated translations via Crowdin
  ...
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Dec 15, 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
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. topic/ui Change the appearance of the Gitea UI
Projects
None yet
Milestone
1.21.0
Development

Successfully merging this pull request may close these issues.
7 participants
@puni9869 @denyskon @silverwind @wxiaoguang @lunny @GiteaBot @delvh