Skip to content

Fix button alignments and remove unnecessary styles #34206

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

Merged
merged 14 commits into from
Apr 16, 2025

Conversation

kerwin612
Copy link
Member

before:
f59655cfe85ec95721537687732bfeb
9e4f723f3bf65dd879123933166fa71

after:
55ee23881e4888c699090041904de6f
c883db1e9b7fb4a6dc358adb1933d69

…a common button group style, and gradually replace the button group styles on other pages later.
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 15, 2025
@github-actions github-actions bot added modifies/templates This PR modifies the template files modifies/frontend labels Apr 15, 2025
@lunny lunny added the topic/ui Change the appearance of the Gitea UI label Apr 15, 2025
@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 Apr 15, 2025
@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 Apr 15, 2025
@silverwind
Copy link
Member

I'd prefer if we just remove margin-right from .ui.button at some point, will be a bit of a refactor, but worth it.

@kerwin612
Copy link
Member Author

kerwin612 commented Apr 15, 2025

I'd prefer if we just remove margin-right from .ui.button at some point, will be a bit of a refactor, but worth it.

Actually, I also agree. However, if we directly delete it from .ui.button, it may affect the usage in other places. So I want to make a mark here to facilitate subsequent maintenance and modification.

Since buttons are not always in groups, there might be a situation where there is another node beside a button, and the margin there might be meaningful. But it's impossible to accurately locate such nodes.

Therefore, my suggestion is that whenever we find such a case, we change it to this mark(gitea-buttons). This will greatly simplify subsequent maintenance and modification.

@silverwind
Copy link
Member

I disagree, we should fix problems at their root instead of creating more technical debt by introducing another class. Also gitea-buttons is a bad name. It tells nothing what it does.

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.

Disagree +1

We already have flex-text-block and flex-text-inline to explicitly use "flex align center with gap", so ideally we need to:

.flex-text-block > .ui.button,
.flex-text-inline > .ui.button {
...
}

And use flex-text-block and flex-text-inline to replace legacy tw-flex styles.

@wxiaoguang wxiaoguang marked this pull request as draft April 15, 2025 09:51
@wxiaoguang wxiaoguang changed the title Fix Release Page Button Style and Extract Common Button Group Style WIP: Fix release page button style and extract common styles Apr 15, 2025
@kerwin612 kerwin612 requested a review from wxiaoguang April 15, 2025 23:58
@wxiaoguang
Copy link
Contributor

We already have flex-text-block and flex-text-inline to explicitly use "flex align center with gap", so ideally we need to:
And use flex-text-block and flex-text-inline to replace legacy tw-flex styles.

Hmm, actually we have already done so in "Adjust the layout of the toolbar on the Issues/Projects page (#33667)"

.flex-text-block > .ui.button,
.flex-text-inline > .ui.button {
  margin: 0; /* fomantic buttons have default margin, when we use them in a flex container with gap, we do not need these margins */
}


.repo-buttons button {
/* ensure that text-btn and svg-text-btn have consistent height. */
line-height: 16px !important;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why it is needed? I don't see difference.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, I can see that it is for the "accept/reject transfer" buttons

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's remove that patch and leave the button height fine-tunes to the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

This patch should also be refactored together later.

image

@wxiaoguang wxiaoguang changed the title WIP: Fix release page button style and extract common styles Fix release page button style and extract common styles Apr 16, 2025
@wxiaoguang wxiaoguang changed the title Fix release page button style and extract common styles Fix release page button style and remove unnecessary styles Apr 16, 2025
@wxiaoguang wxiaoguang marked this pull request as ready for review April 16, 2025 00:58
@wxiaoguang wxiaoguang force-pushed the fix/release-btn-style branch from 077fb57 to 3d7db9f Compare April 16, 2025 03:33
@wxiaoguang wxiaoguang changed the title Fix release page button style and remove unnecessary styles Fix button aligments and remove unnecessary styles Apr 16, 2025
@wxiaoguang wxiaoguang changed the title Fix button aligments and remove unnecessary styles Fix button alignments and remove unnecessary styles Apr 16, 2025
@wxiaoguang wxiaoguang force-pushed the fix/release-btn-style branch from 3d7db9f to a0150b7 Compare April 16, 2025 03:38
@wxiaoguang
Copy link
Contributor

Removed more hacky patches.

@wxiaoguang wxiaoguang force-pushed the fix/release-btn-style branch from a0150b7 to 97b4eef Compare April 16, 2025 03:50
@wxiaoguang wxiaoguang force-pushed the fix/release-btn-style branch from 97b4eef to e9bcc17 Compare April 16, 2025 03:54
@kerwin612 kerwin612 added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Apr 16, 2025
@GiteaBot
Copy link
Collaborator

@kerwin612 please fix the merge conflicts. 🍵

@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Apr 16, 2025
@wxiaoguang wxiaoguang enabled auto-merge (squash) April 16, 2025 07:44
@wxiaoguang wxiaoguang added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Apr 16, 2025
@wxiaoguang wxiaoguang merged commit bde014e into go-gitea:main Apr 16, 2025
26 checks passed
@GiteaBot GiteaBot added this to the 1.25.0 milestone Apr 16, 2025
@kerwin612 kerwin612 deleted the fix/release-btn-style branch April 16, 2025 08:20
@lunny lunny modified the milestones: 1.25.0, 1.24.0 Apr 16, 2025
zjjhot added a commit to zjjhot/gitea that referenced this pull request Apr 18, 2025
* giteaofficial/main:
  Fix various misalignments and overflows (go-gitea#34227)
  Update net package (go-gitea#34228)
  Fix button alignments and remove unnecessary styles (go-gitea#34206)
  Fix project board links to related Pull Requests (go-gitea#34213)
  Fix empty repo clone panel border (go-gitea#34219)
  Fix: quoted replies incorrectly render user input as part of the quote (go-gitea#34216)
lunny added a commit that referenced this pull request Apr 27, 2025
Continue with #34206.

---------

Co-authored-by: wxiaoguang <[email protected]>
Co-authored-by: Lunny Xiao <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/frontend modifies/templates This PR modifies the template files topic/ui Change the appearance of the Gitea UI type/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants