Skip to content

Fix regression with action button height with icon #26850

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

Closed
wants to merge 1 commit into from

Conversation

silverwind
Copy link
Member

@silverwind silverwind commented Aug 31, 2023

One of the recent changes seems to have removed this needed CSS rule, resulting in this regression where the input element is stretched too high because of the SVG:

Before (search bar too high):
Screenshot 2023-08-31 at 23 13 22

After (search roughly equal in height to buttons on left):
Screenshot 2023-08-31 at 23 13 31

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Aug 31, 2023
@silverwind silverwind added the skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. label Aug 31, 2023
@delvh
Copy link
Member

delvh commented Aug 31, 2023

@wxiaoguang

@wxiaoguang
Copy link
Contributor

Thank you, didn't find it. Maybe it need a comment about which page uses it

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Sep 1, 2023

Tested, I think the style could be replaced by padding-top/bottom: 0:

Maybe:

/* let the button's flex layout the icon, no need extra y-padding */
.ui.action.input .ui.icon.button {
    padding-top: 0;
    padding-bottom: 0;
}

image

@silverwind
Copy link
Member Author

I guess we can simplify the selector and call it out in the CSS. Maybe I'll also try a general solution to buttons with svg.

@silverwind silverwind changed the title Fix regression with action button with icon Fix regression with action button height with icon Sep 1, 2023
@wxiaoguang
Copy link
Contributor

I have a feeling that the padding-top/bottom: 0 could be added to .ui.action.input .button (line 487)

Now the buttons all have display: flex/inline-flex and align-items: center, the action input button doesn't need padding-top/bottom any more, they could just share the container's height and layout correctly.

@techknowlogick techknowlogick added topic/ui Change the appearance of the Gitea UI issue/regression Indicates a previously functioning feature or behavior that has broken or regressed after a change labels Sep 1, 2023
@wxiaoguang
Copy link
Contributor

-> Remove CSS has selector and improve various styles #26891

lunny pushed a commit that referenced this pull request Sep 4, 2023
Replace  #26850

Major changes:

1. Remove all `has` selectors, it is still not supported by firefox.
Actually there could be some more general and clearer approaches
2. Remove `two-toggle-buttons`, the `.ui.buttons` just works well
3. Rewrite the `.ui.buttons` border styles, see the screenshots
4. Remove the "fine-tuning" paddings from the the flex children, they
could layout themselves well.

![image](https://github.com/go-gitea/gitea/assets/2114189/a32ed6f3-60f7-43d5-9492-62c45d2397f6)

![image](https://github.com/go-gitea/gitea/assets/2114189/5cb173c5-c942-4237-8cb4-2697220b3f06)

![image](https://github.com/go-gitea/gitea/assets/2114189/8a1c12b3-a632-48ff-b1a7-a01a4417f821)

![image](https://github.com/go-gitea/gitea/assets/2114189/46bde1bd-9113-4231-965d-6ec9076f6a3b)
@silverwind
Copy link
Member Author

silverwind commented Sep 4, 2023

I'm skeptical whether :has could be removed in all cases, but we'll see.

@silverwind silverwind closed this Sep 4, 2023
@silverwind silverwind deleted the searchheight branch September 4, 2023 16:33
@wxiaoguang
Copy link
Contributor

I have tested all the related UI, except one:

/* fix fomantic's border-radius via :first-child with hidden elements */
.collapsible-comment-box:has(.gt-hidden) {
  border-radius: var(--border-radius) !important;
}

Actually I didn't see difference after removing it. If it doesn't look good, feel free to let me know.

@silverwind
Copy link
Member Author

silverwind commented Sep 4, 2023

It is this box and yes, it has regressed:

https://try.gitea.io/silverwind/symlink-test/pulls/2

image

Notice the background bleeding into the border on bottom. The hiding mechanism is crude and just hides all children with gt-hidden, so ::last-child does not work. It could be fixed by adding decicated classname when collapsed.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Sep 4, 2023

I think this style is good enough, no trick.

class="collapsible-comment-box gt-p-3 gt-df gt-ac gt-sb"

image

image

@wxiaoguang
Copy link
Contributor

Or, keep using "ui segment" for consistency, and use:

class="ui segment collapsible-comment-box gt-my-3 gt-py-0 gt-df gt-ac gt-sb"

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Sep 4, 2023

Or, make it (collapsible-comment-box) have background: transparent;

class="ui segment collapsible-comment-box gt-bg-transparent gt-py-3 gt-df gt-ac gt-sb"

@silverwind
Copy link
Member Author

Works for me, I wouldn't mind removal of ui segment, the less fomantic, the better.

@silverwind
Copy link
Member Author

I guess the use case kind of screams for a details element which wouldn't need JS to toggle.

@go-gitea go-gitea locked as resolved and limited conversation to collaborators Dec 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
issue/regression Indicates a previously functioning feature or behavior that has broken or regressed after a change lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. 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
Development

Successfully merging this pull request may close these issues.

5 participants