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

Refactor hiding-methods, remove jQuery show/hide, remove .hide class, remove inline style=display:none #22950

Merged
merged 7 commits into from
Feb 19, 2023

Conversation

wxiaoguang
Copy link
Contributor

@wxiaoguang wxiaoguang commented Feb 17, 2023

Close #22847

This PR:

  • introduce Gitea's own showElem and related functions
  • remove jQuery show/hide
  • remove .hide class
  • remove inline style=display:none

From now on:

do not use:

  • "[hidden]" attribute: it's too weak, can not be applied to an element with "display: flex"
  • ".hidden" class: it has been polluted by Fomantic UI in many cases
  • inline style="display: none": it's difficult to tweak
  • jQuery's show/hide/toggle: it can not show/hide elements with "display: xxx !important"

only use:

  • this ".gt-hidden" class
  • showElem/hideElem/toggleElem functions in "utils/dom.js"

cc: @silverwind , this is the all-in-one PR

@lunny lunny added topic/ui Change the appearance of the Gitea UI type/refactoring Existing code has been cleaned up. There should be no new functionality. labels Feb 17, 2023
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 17, 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 Feb 17, 2023
@silverwind
Copy link
Member

This does look promising, hope it doesn't break stuff :D

@wxiaoguang
Copy link
Contributor Author

wxiaoguang commented Feb 17, 2023 via email

@wxiaoguang
Copy link
Contributor Author

wxiaoguang commented Feb 17, 2023 via email

@silverwind
Copy link
Member

silverwind commented Feb 17, 2023

Why?

When all show/hide mechanism rely on class name, we don't need the expensivegetComputedStyle and can just check class existance.

Move to where?

Move the window.config.runModeIsProd check to the call site.

@wxiaoguang
Copy link
Contributor Author

wxiaoguang commented Feb 17, 2023

Why?

When all show/hide mechanism rely on class name, we don't need the expensivegetComputedStyle and can just check class existance.

No, that's not what it works.

getComputedStyle is only a fallback (debug assertion) check to detect whether there are something like style=display:none or .popup {display:none}, to catch bugs as early as possible.

You can not replace it with contains.

Move to where?

Move the window.config.runModeIsProd check to the call site.

It will make code quite complex and dirty (you can have a try).

And see the comment: "this assertion can be removed after next release cycle or if it has been proved that there is no bug."

@silverwind
Copy link
Member

OK I guess if that function is not here forever, it's better to keep the check inside it.

web_src/js/utils/dom.js Show resolved Hide resolved
web_src/js/utils/dom.js Show resolved Hide resolved
web_src/js/utils/dom.js Show resolved Hide resolved
web_src/js/features/repo-legacy.js Show resolved Hide resolved
Copy link
Member

@delvh delvh left a comment

Choose a reason for hiding this comment

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

Found no errors during my testing.

@GiteaBot GiteaBot removed the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Feb 18, 2023
@wxiaoguang wxiaoguang deleted the refactor-hide branch February 19, 2023 09:16
zjjhot added a commit to zjjhot/gitea that referenced this pull request Feb 20, 2023
* upstream/main:
  Fix broken pull request files (go-gitea#22962)
  Fix avatar misalignment (go-gitea#22955)
  Refactor the setting to make unit test easier (go-gitea#22405)
  Migration v244.go should be v243.go (go-gitea#22988)
  Adjust manifest to prevent tagging latest on rcs (go-gitea#22811)
  Add some guidelines for refactoring (go-gitea#22880)
  Rename `GetUnits` to `LoadUnits` (go-gitea#22970)
  Provide the ability to set password hash algorithm parameters (go-gitea#22942)
  Fix no user listed in org teams page (go-gitea#22979)
  Refactor hiding-methods, remove jQuery show/hide, remove `.hide` class, remove inline style=display:none (go-gitea#22950)
  Scoped labels (go-gitea#22585)
  Rename "People" to "Members" in organization page and use a better icon (go-gitea#22960)
  Rename `repo.GetOwner` to `repo.LoadOwner` (go-gitea#22967)
  Notify on container image create (go-gitea#22806)
  webview: Fix overflowing diff body (go-gitea#22959)
  Introduce customized HTML elements, fix incorrect AppUrl usages in templates (go-gitea#22861)
  Sort issues and pulls by recently updated in user and organization home (go-gitea#22925)
  Fix 404 error viewing the LFS file (go-gitea#22945)
brechtvl added a commit to brechtvl/gitea that referenced this pull request Feb 20, 2023
jolheiser pushed a commit that referenced this pull request Feb 20, 2023
Follows:
* #22950

The dropdown menu works well without these codes.

The reason is that the event bubbling still works for the dropdown menu,
the Fomantic UI dropdown menu module will hide the menu correctly if an
item is clicked.
lunny pushed a commit that referenced this pull request Feb 21, 2023
This PR follows: 
* #22950

### Before

The Review Box has many problems:

* It doesn't work for small screens.
* It has an anonying animation which makes the UI laggy.
* It uses "custom dropdown menu" which is very difficult to fine tune.
* `$().toggle('visible')` is not a correct call 
* jQuery just accepts any invalid `duration` argument:
`$().toggle('anyting')`
* The button is not a button.

<details>

![image](https://user-images.githubusercontent.com/2114189/219948865-6da3f39c-6fde-4c86-9e42-da5020f3d0c3.png)

</details>

### After

These problems are fixed, and eliminate many `!important` games.

<details>

![image](https://user-images.githubusercontent.com/2114189/219952744-8862fe1a-7ef1-49e4-bf92-2d0c1f104ee4.png)

![image](https://user-images.githubusercontent.com/2114189/219952771-be169a76-45fd-47a8-8f9c-b447d064f4ca.png)

![image](https://user-images.githubusercontent.com/2114189/219952784-3f52e9b7-64ce-4ad1-9553-64c33fb83042.png)

</details>

And most dropdown icons still looks good:

<details>

![image](https://user-images.githubusercontent.com/2114189/219952942-52866a00-e0f9-4af7-8fb5-eb1a8cad1ff3.png)

![image](https://user-images.githubusercontent.com/2114189/219948909-b3bfb844-f84e-4b79-ab1f-382ec66dec31.png)

</details>

Co-authored-by: delvh <leon@kske.dev>
zeripath pushed a commit that referenced this pull request Feb 22, 2023
@yp05327 yp05327 mentioned this pull request Mar 1, 2023
lunny pushed a commit that referenced this pull request Mar 2, 2023
#22950 removed `hide` class, and
use `gt-hidden`
But there are some missed `hide`....

---------

Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
GiteaBot pushed a commit to GiteaBot/gitea that referenced this pull request Mar 2, 2023
go-gitea#22950 removed `hide` class, and
use `gt-hidden`
But there are some missed `hide`....

---------

Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
jolheiser pushed a commit that referenced this pull request Mar 2, 2023
Backport #23208

#22950 removed `hide` class, and
use `gt-hidden`
But there are some missed `hide`....

Co-authored-by: yp05327 <576951401@qq.com>
techknowlogick pushed a commit that referenced this pull request Apr 2, 2023
When doing the refactoring:

* #22950

I added some debug mode code (assertShown) to help to catch bugs, it did
catch some bugs like:

* #23074


If it has been proved that there is no more bugs, this assertion could
be removed easily and clearly.

Feel free to decide when to remove it (feel free to convert it from
Draft to Ready for Review).


cc: @silverwind
GiteaBot pushed a commit to GiteaBot/gitea that referenced this pull request Apr 2, 2023
When doing the refactoring:

* go-gitea#22950

I added some debug mode code (assertShown) to help to catch bugs, it did
catch some bugs like:

* go-gitea#23074


If it has been proved that there is no more bugs, this assertion could
be removed easily and clearly.

Feel free to decide when to remove it (feel free to convert it from
Draft to Ready for Review).


cc: @silverwind
lunny pushed a commit that referenced this pull request Apr 2, 2023
Backport #23576 by @wxiaoguang

When doing the refactoring:

* #22950

I added some debug mode code (assertShown) to help to catch bugs, it did
catch some bugs like:

* #23074

If it has been proved that there is no more bugs, this assertion could
be removed easily and clearly.

Feel free to decide when to remove it (feel free to convert it from
Draft to Ready for Review).
@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. topic/ui Change the appearance of the Gitea UI type/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Proposal] Refactor frontend hide/hidden mechanism
6 participants