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

Move repo setting menu to right side to keep consistent with org's #28976

Closed
wants to merge 3 commits into from
Closed

Move repo setting menu to right side to keep consistent with org's #28976

wants to merge 3 commits into from

Conversation

lunny
Copy link
Member

@lunny lunny commented Jan 29, 2024 •
edited
Loading

Before

图片

After

图片

For mobile, it's the same as before.
图片

@lunny lunny added topic/ui Change the appearance of the Gitea UI skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. labels Jan 29, 2024
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jan 29, 2024
@pull-request-size pull-request-size bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jan 29, 2024
@github-actions github-actions bot added the modifies/templates This PR modifies the template files label Jan 29, 2024
@lunny lunny requested a review from denyskon January 29, 2024 15:52
@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 Jan 29, 2024
denyskon
denyskon suggested changes Jan 29, 2024
View reviewed changes
Copy link
Member

@denyskon denyskon left a comment

Choose a reason for hiding this comment

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

Have to check how it looks on mobile...

@GiteaBot GiteaBot added lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jan 29, 2024
@silverwind
Copy link
Member

silverwind commented Jan 29, 2024 •
edited
Loading

It bleeds into the fade-out effect at the end of the tab bar, which is imho unacceptable.

We should replace this fade out effect with a GitHub-like mechanism that moves overflowing tab bar elements into a ... button, but that will require some JS to work. With that, Settings can only stay left-aligned.

@yardenshoham yardenshoham self-requested a review January 29, 2024 17:25
@lunny lunny changed the title Move repo setting mean to right side to keep consistent with org's Move repo setting menu to right side to keep consistent with org's Feb 2, 2024
@lunny
Copy link
Member Author

lunny commented Feb 2, 2024

Have to check how it looks on mobile...

I have checked it works. See the updated screenshots.

@lunny
Copy link
Member Author

lunny commented Feb 25, 2024

But now organization's settings are already aligned to the right. This PR will keep them both consistent.

wxiaoguang
wxiaoguang approved these changes Feb 25, 2024
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.

I think it doesn't really cause any problem. If any, it could still be reverted or fixed easily.

@lunny
Copy link
Member Author

lunny commented Feb 25, 2024

Have to check how it looks on mobile...

Mobile screenshots updated. It's not affected.

@lunny lunny removed the lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged label Feb 25, 2024
@lunny lunny added this to the 1.22.0 milestone Feb 25, 2024
@lunny lunny requested a review from denyskon February 25, 2024 05:26
@GiteaBot GiteaBot added the lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged label Feb 25, 2024
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged labels Feb 25, 2024
@yardenshoham yardenshoham added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Feb 25, 2024
@silverwind
Copy link
Member

silverwind commented Feb 25, 2024 •
edited
Loading

It does affect desktop where the "s" in "Settings" is faded out as well as the line below. Thise fade-out is not present on org page. Imho it looks to bad and I have to block it.

silverwind
silverwind requested changes Feb 25, 2024
View reviewed changes
Copy link
Member

@silverwind silverwind left a comment

Choose a reason for hiding this comment

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

Does not work well for repo because of fade-out effect which is not present on org.

@GiteaBot GiteaBot added lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged and removed lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. labels Feb 25, 2024
@silverwind
Copy link
Member

silverwind commented Feb 25, 2024 •
edited
Loading

Removing the fade-out on this new-menu might be one way to solve it that is borderline acceptable to me. A better way would be to implement a tab bar that overflows into a menu like this, ideally using a web component to prevent loading flash.

@silverwind silverwind removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Feb 25, 2024
@silverwind
Copy link
Member

I will have a try at implementing such an overflow menu. Won't be a web component at least, though.

@silverwind silverwind mentioned this pull request Feb 25, 2024
Merged
5 tasks
@silverwind
Copy link
Member

Depends on #29400 and I will implement the moving of the settings menu in it I think.

@lunny lunny closed this Mar 3, 2024
@lunny lunny deleted the lunny/repo_setting_right branch March 3, 2024 02:14
@GiteaBot GiteaBot removed this from the 1.22.0 milestone Mar 3, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@silverwind silverwind silverwind requested changes

@denyskon denyskon denyskon approved these changes

@wxiaoguang wxiaoguang wxiaoguang approved these changes

@yardenshoham yardenshoham Awaiting requested review from yardenshoham

Assignees
No one assigned
Labels
lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged modifies/templates This PR modifies the template files size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. topic/ui Change the appearance of the Gitea UI
Projects
None yet
Milestone
No milestone
Development

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