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

Use separate class for tippy targets #20742

Merged
merged 2 commits into from
Aug 10, 2022
Merged

Conversation

silverwind
Copy link
Member

@silverwind silverwind commented Aug 9, 2022

Previous solution that re-purposed the 'hide' class by making it !important had various unintended side-effects where jQuery .show() was not able to outweight it. Use a separate class to prevent these interactions.

Fixes: #20747
Regressed by: #20428

@silverwind silverwind added this to the 1.18.0 milestone Aug 9, 2022
@silverwind silverwind added the topic/ui Change the appearance of the Gitea UI label Aug 9, 2022
@silverwind
Copy link
Member Author

silverwind commented Aug 9, 2022

Note for future refactor: Enable jquery/no-hide and jquery/no-show linter rules and replace cases with helper function that toggles the hide class instead. This also brings the benefit of not forcing display: block on shown elements.

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

wxiaoguang commented Aug 10, 2022

I would strongly suggest to revert the !important for the .hide CSS class at the moment.

Many things are broken, there may be more:

The show/hide game has been very complex in Gitea. IIRC there are .hide, .hidden, :hidden, display: none, $.hide(), etc.

I would suggest to think about a more more comprehensive plan and resolve the problem fundamentally. At least, there should be a document about what should be done while what shouldn't.

@silverwind
Copy link
Member Author

silverwind commented Aug 10, 2022

Problem was without the !important, the popover targets in DOM would flash until tippy loads and removes them from DOM. Previously there was some !important class coming from Fomantic, but as that module was removed, I had to rely on existing .hide which has caused side effects.

Options to fix is either like this PR to fix up all cases of .hide to just toggle that class or introduce another hide class specific for popover targets. The latter would certainly be a lot easier, but it again adds complexity.

@silverwind
Copy link
Member Author

silverwind commented Aug 10, 2022

The issue with the comment edit form is that it uses $.show() which inserts inline styles the previously outweighted .hide by being inline and therefore higher precedence. The .hide class stayed on those elements all the time:

image

I think the better solution is just to remove .hide to show content. At least to me it's a more obvious and lightweight way than to rely on inline styles and it will help reduce our dependency on jQuery-specific stuff like that.

@silverwind
Copy link
Member Author

silverwind commented Aug 10, 2022

On the other hand, there's just too much stuff that relies on $.hide, I don't think I'm willing to refactor/test all this mess currently, so I think I will go the easy solution and add a separate class to hide popover targets during page load.

Previous solution that re-purposed the 'hide' class by making it
!important had various unintended side-effects where jQuery .show() was
not able to outweight it. Use a separate class to prevent these
interactions.

Fixes: go-gitea#20747
@wxiaoguang
Copy link
Contributor

Personally I would always avoid !important as much as possible, I treat it as "evil".

I could have the willing to refactor existing code, but I would like to see some agreements and feasible plans first.

And, the most important thing is, the bugs caused by the .hide should be fixed soon. 😁

@silverwind silverwind changed the title Fix show-panel mechanism Use separate class for tippy targets Aug 10, 2022
@silverwind
Copy link
Member Author

!important is a necessary evil when not all CSS is first-party, but even then it's often just very useful to sometimes "skip" the CSS precedence rules.

I updated the PR now to use the separate class approach.

Refactor should be pretty simple in theory, replace all .hide() with .addClass('hide') and all .show() with .removeClass('hide') while also replacing all inline style="display: none" HTML templates with the addtion of the hide class.

@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 Aug 10, 2022
@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 Aug 10, 2022
@wxiaoguang wxiaoguang linked an issue Aug 10, 2022 that may be closed by this pull request
@wxiaoguang wxiaoguang added the skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. label Aug 10, 2022
@wxiaoguang wxiaoguang merged commit d751e35 into go-gitea:main Aug 10, 2022
zjjhot added a commit to zjjhot/gitea that referenced this pull request Aug 10, 2022
* giteaofficial/main:
  Use separate class for tippy targets (go-gitea#20742)
  Add support mCaptcha as captcha provider (go-gitea#20458)
@silverwind silverwind deleted the fixhide branch August 10, 2022 16:23
vsysoev pushed a commit to IntegraSDL/gitea that referenced this pull request Aug 28, 2022
Previous solution that re-purposed the 'hide' class by making it
`!important` had various unintended side-effects where jQuery .show() was
not able to outweight it. Use a separate class to prevent these
interactions.
@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/done This PR has enough approvals to get merged. There are no important open reservations anymore. 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 type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Edit form doesn't display when editing issue/comment
4 participants