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

Fix display for context menu #23342

Closed
wants to merge 2 commits into from
Closed

Conversation

HesterG
Copy link
Contributor

@HesterG HesterG commented Mar 7, 2023

This PR is to fix the wrong display for context menu. The reason this occurs is because display was overwitten by style of fomantic component, specifically the following rule

.ui.comments .comment .actions a {
    cursor: pointer;
    display: inline-block; // should be block
    margin: 0 0.75em 0 0;
    color: rgba(0, 0, 0, 0.4);
}

Before:
截屏2023-03-07 08 54 28

After:

截屏2023-03-07 08 50 45

@lunny lunny added the type/bug label Mar 7, 2023
@lunny lunny added this to the 1.20.0 milestone Mar 7, 2023
@@ -1,6 +1,7 @@
.gt-df { display: flex !important; }
.gt-di { display: inline !important; }
.gt-dif { display: inline-flex !important; }
.gt-db { display: block !important; }
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

I think HesterG's answer is definitely yes because she already gave your proposal a thumbs up. 😄

But maybe it's a good idea to keep the old style before refactoring?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you think about this proposal:

I pretty agreed with this comment, but i am note sure if we could partially introduce tailwind, like only introduce widely used css like we now have inside helper.less

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 7, 2023
@wxiaoguang
Copy link
Contributor

I think the root problem is aria.js is too hacky, we can avoid using a here, I will propose new PR to avoid making the problem more complex.

@wxiaoguang
Copy link
Contributor

See Fix incorrect display for comment context menu #23343

Actually I had a TODO there "in the future there could be a special CSS class for it". that TODO would help similar problems in the future.

@HesterG
Copy link
Contributor Author

HesterG commented Mar 7, 2023

Closed as #23343 fixed the root problem

@HesterG HesterG closed this Mar 7, 2023
@lunny lunny removed this from the 1.20.0 milestone Mar 7, 2023
lunny pushed a commit that referenced this pull request Mar 8, 2023
Replace #23342 

Fix a regression of #23014: the `a` couldn't be used here because
Fomantic UI has style conflicts: `.ui.comments .comment .actions a {
display: inline-block; }`


And complete one more of my TODOs: "in the future there could be a
special CSS class for it"
@HesterG HesterG deleted the fix-menu-display branch March 15, 2023 03:15
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants