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.

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

[UI] Fix misalligned icons #25158

Closed
wants to merge 2 commits into from

Conversation

earl-warren
Copy link
Contributor

@earl-warren earl-warren commented Jun 8, 2023

- On the pull request page, two icons were misaligned vertically with
their text part.
- This patch adds the simple flexbox trick with `align-items: center` to
vertically center the children elements and adds `gt-ml-2` to the text,
to add space between icon and text that would otherwise be removed
because of `display: flex`.
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jun 8, 2023
@pull-request-size pull-request-size bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jun 8, 2023
@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 Jun 8, 2023
@wxiaoguang
Copy link
Contributor

Keeping adding "gt-df gt-ac" is not the right approach. If you search <i class="icon icon-octicon"> , there are a lot.

So, I do not think it's urgent to introduce this patch, I will propose a complete fix for SVG alignment.

@silverwind
Copy link
Member

Keeping adding "gt-df gt-ac" is not the right approach. If you search <i class="icon icon-octicon"> , there are a lot.

So, I do not think it's urgent to introduce this patch, I will propose a complete fix for SVG alignment.

I'm okay with it. As I said earlier, SVG vertical centering with text has no univeral fix that works for every font and icon size.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Jun 8, 2023

SVG vertical centering with text has no univeral fix that works for every font and icon size.

I think we can introduce a helper CSS class like this:

<button class="with-flex-icon">
   <svg ...>
   text ...
   <svg ...>
   text
   <svg ...>
</button>

Then let the first "svg" get margin-right, the last "svg" get "margin-left", inside "svg" gets margin-left&margin-right.

And we need to remove all "icon icon-oction" from code, it's a no-op IMO (IIRC the fomantic icon module had been removed)

@silverwind
Copy link
Member

silverwind commented Jun 8, 2023

I think we can introduce a helper CSS class like this:

I don't mind the explicit gt-df gt-ac so much, they are true to CSS so to say and their meaning is immediately clear, which is not the case with a with-flex-icon that one has to first lookup in the CSS for what magic it does.

And we need to remove all "icon icon-oction" from code, it's a no-op IMO.

Agree, <i> wrappers are unnecessary, except for the cases where Fomantic has explicit selectors for it, which it does for i.icon in some cases.

@wxiaoguang
Copy link
Contributor

I think we can introduce a helper CSS class like this:

I don't mind the explicit gt-df gt-ac so much, they are true to CSS so to say and their meaning is immediately clear, which is not the case with a with-flex-icon that one has to first lookup in the CSS for what magic it does.

with-flex-icon is just an example. For detailed examples:

<button class="ui button gt-df gt-ac">
    {{svg "name"}}
    <span class="gt-ml-2">title</span>
</button>

or

<button class="ui button gt-df gt-ac">{{svg "name"}}<span class="gt-ml-2">title</span></button>

VS

<button class="ui button flex-icon">{{svg "name"} title</button>

I think the second one is better, and we can simply show it in the devtest page to let developers copy and reuse the code.

And one more thing, there are still a lot of dropdowns, some of them still have problem with the arrow icon.

@silverwind
Copy link
Member

I do believe this is not a complete fix for alle the messages that can appear in that box:

image

@earl-warren can you give the other messages the same treatment?

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Jun 8, 2023

I do believe this is not a complete fix for alle the messages that can appear in that box:

That's what I said "there are a lot" ..... I believe this problem needs a framework level support.

@silverwind
Copy link
Member

I don't agree with introducing magical classes, they will hurt in the long run once requirements get more complex than "a button with an icon", just use the simple CSS helpers in these cases for now please.

@wxiaoguang
Copy link
Contributor

Sorry I don't understand the point"magical classes". If such design is called "magical", then every modern framework is full of magical classes. Are they all wrong?

@silverwind
Copy link
Member

silverwind commented Jun 8, 2023

It depends, common classes can help when the use case can be clearly defined, but one must not make it excessive (like Fomantic). I do see a possibility of having a text-with-icons class helper, equivalent to gt-df gt-ac gt-fw gt-gap-2.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Jun 9, 2023

What do you think about: Use flex to align SVG and text #25163 ?

@earl-warren
Copy link
Contributor Author

Closing in favor of #25163

@wxiaoguang
Copy link
Contributor

Keeping adding "gt-df gt-ac" is not the right approach. If you search <i class="icon icon-octicon"> , there are a lot.

So, I do not think it's urgent to introduce this patch, I will propose a complete fix for SVG alignment.

The complete fix is ready: Use flex to align SVG and text #25163

@go-gitea go-gitea locked as resolved and limited conversation to collaborators Sep 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 1 This PR needs approval from one additional maintainer to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants