Skip to content

Conversation

@silverwind
Copy link
Member

@silverwind silverwind commented Nov 6, 2025

Contains a number of minor CSS fixes.

Fix missing border on targeted speech bubble
Screenshot 2025-11-06 at 22 43 31

Add padding to inline comments, slightly more padding around emoji button
Screenshot 2025-11-06 at 22 38 39

Center text on header in code search results
Screenshot 2025-11-06 at 22 08 01

Tweak emoji selector, reducing font size primarily
Screenshot 2025-11-06 at 22 29 46

Minor tweaks to repo sidebar, reduce font size by 1px, center "Release" text with label.
image

Fix issue comment buttons being misaligned on mobile
Screenshot 2025-11-06 at 22 50 19

Add highlight to actions re-run icon
Screenshot 2025-11-06 at 23 04 30

Fix actions re-run button overflow
image

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Nov 6, 2025
@github-actions github-actions bot added modifies/templates This PR modifies the template files modifies/frontend labels Nov 6, 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 Nov 6, 2025
silverwind and others added 2 commits November 7, 2025 11:27
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
Signed-off-by: silverwind <me@silverwind.io>
@silverwind
Copy link
Member Author

One more fix added to improve the issue timeline:

Before (content breaks out of box):
Screenshot 2025-11-07 at 13 24 18

After (content is wrapped and slightly more compact):
Screenshot 2025-11-07 at 13 23 57

@wxiaoguang

This comment was marked as resolved.

@wxiaoguang

This comment was marked as resolved.

@wxiaoguang wxiaoguang marked this pull request as draft November 7, 2025 12:52
@wxiaoguang

This comment was marked as resolved.

@silverwind
Copy link
Member Author

This damn history is so finicky, thanks for catching.

@wxiaoguang
Copy link
Contributor

This damn history is so finicky, thanks for catching.

Because there are so many hacky patches, unclear names, style conflicts.

@silverwind
Copy link
Member Author

silverwind commented Nov 7, 2025

Yes, it needs a redesign. I've reverted part of the changes, now it's only doing the wrapping (padding so it does not wrap exactly at element right side):

image

Line height is way too much here but I see no easy fix. The old code never assumed text to wrap.

@wxiaoguang
Copy link
Contributor

Line height is way too much here but I see no easy fix. The old code never assumed text to wrap.

Interesting, the regressions are all related to that problematic "line-height" 🤣

@@ -1,5 +1,5 @@
{{if ctx.RootData.IsSigned}}
<div class="item action ui dropdown jump pointing top right select-reaction" data-action-url="{{.ActionURL}}">
<div class="item action ui dropdown jump pointing top right select-reaction tw-px-[5px]" data-action-url="{{.ActionURL}}">
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 has 5px padding but the other one doesn't:

image image

Copy link
Member Author

Choose a reason for hiding this comment

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

I wanted to add a bit more spacing around the emoji button, but at the same time not introduce too much spacing between the labels. I guess the clean solution is to put the labels into their own <div> and apply different gaps.

Copy link
Contributor

Choose a reason for hiding this comment

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

By the way, the UI has different cases.

On "diff" page, the layout is like this, it doesn't show "role" labels, but "review" or "pending" labels.

And the "emoji" dropdown sometimes can be hidden templates/repo/diff/comments.tmpl.

I think we need a unified UI layout to cover these cases.

image

line-height: 32px;
vertical-align: middle;
color: var(--color-text-light);
overflow-wrap: break-word;
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? IIRC break-word is already inherited from body

Copy link
Member Author

Choose a reason for hiding this comment

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

A unbroken string of text did not wrap without it. Guess I will check what can be done with inheritance.

Comment on lines +535 to +536
max-width: 100%;
padding-right: 1rem;
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 needs these two new styles?

I don't see difference.

Copy link
Member Author

@silverwind silverwind Nov 7, 2025

Choose a reason for hiding this comment

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

Without it, a unbroken line will wrap exactly at the end of the element, which looks visually unpleasing without any spacing.

Copy link
Contributor

Choose a reason for hiding this comment

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

  • padding-right: 1rem;: add extra space for wrapped content?
  • max-width: 100%;: what does this do?

background: transparent;
border-bottom: 0;
padding: 0;
min-height: auto;
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe the min-height: 41px should be removed from .comment-header, then no need to add a new min-height: auto; here?

Copy link
Member Author

Choose a reason for hiding this comment

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

If the header of regular comments can naturally size to 41px, then yes, this woul not be needed.

Copy link
Contributor

@wxiaoguang wxiaoguang Nov 7, 2025

Choose a reason for hiding this comment

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

The "min-height: 41px" is from #13463

Do you still remember the reason, maybe it is used to align the triangles? Maybe it's better to add some comments.

And the min-height: 41px can be only applied to .comment-header.avatar-content-left-arrow

border-bottom: 0;
padding: 0;
min-height: auto;
margin-bottom: 4px;
Copy link
Contributor

Choose a reason for hiding this comment

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

The margin can be provided by .repository.view.issue .comment-list .code-comment .comment-content, it already has other margin spaces.

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe, this margin is just to introduce a bit more whitespace between comment and header. Maybe a gap will also work.

Comment on lines +657 to +658
margin: 0 -4px;
padding: 8px;
Copy link
Contributor

Choose a reason for hiding this comment

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

The spaces are provided by .repository.view.issue .comment-list .ui.comments, so I think we should avoid the new negative margins.

Copy link
Member Author

Choose a reason for hiding this comment

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

Idk, the negative margin allows the item to "hang" slightly outside which is nice. Mabye there are better solutions, the current spacing is optimal imho.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's a good time to clean up legacy style conflicts, then it will be easier to make more UI improvements.

And we don't need to fully backport these changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport/v1.25 lgtm/need 1 This PR needs approval from one additional maintainer to be merged. modifies/frontend modifies/templates This PR modifies the template files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants