-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Conversation
08589a4
to
a347fab
Compare
a347fab
to
d132fb0
Compare
This does look promising, hope it doesn't break stuff :D |
Why?
silverwind ***@***.***>
… ***@***.**** commented on this pull request.
------------------------------
In web_src/js/utils/dom.js
<#22950 (comment)>:
> @@ -0,0 +1,65 @@
+function getComputedStyleProperty(el, prop) {
+ const cs = el ? window.getComputedStyle(el) : null;
+ return cs ? cs[prop] : null;
+}
+
+function isShown(el) {
+ return getComputedStyleProperty(el, 'display') !== 'none';
+}
classList.contains?
—
Reply to this email directly, view it on GitHub
<#22950 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAQEFDKB3VONTDB264N4UVDWX4YOHANCNFSM6AAAAAAU7BAESI>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Move to where?
silverwind ***@***.***>
… ***@***.**** commented on this pull request.
------------------------------
In web_src/js/utils/dom.js
<#22950 (comment)>:
> @@ -0,0 +1,65 @@
+function getComputedStyleProperty(el, prop) {
+ const cs = el ? window.getComputedStyle(el) : null;
+ return cs ? cs[prop] : null;
+}
+
+function isShown(el) {
+ return getComputedStyleProperty(el, 'display') !== 'none';
+}
+
+function assertShown(el, expectShown) {
+ if (window.config.runModeIsProd) return;
Better to move this outside for a tiny perf gain as JS does not have to
call the function every time.
—
Reply to this email directly, view it on GitHub
<#22950 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAQEFDPS5BQ5YH55SQYBNPTWX4YSJANCNFSM6AAAAAAU7BAESI>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
When all show/hide mechanism rely on class name, we don't need the expensive
Move the |
No, that's not what it works.
You can not replace it with
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." |
OK I guess if that function is not here forever, it's better to keep the check inside it. |
There was a problem hiding this 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.
* 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)
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.
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>
go-gitea#22950 removed `hide` class, and use `gt-hidden` But there are some missed `hide`.... --------- Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
Replace #23310, Close #19733 And fix various UI problems, including regressions from #22959 #22950 and more. ## SVG Detection The old regexp may mismatch non-SVG files. This PR adds new tests for those cases. ## UI Changes ### Before ![image](https://user-images.githubusercontent.com/2114189/222967716-f6ad8721-f46a-4a3f-9eb0-a89e488d3436.png) ![image](https://user-images.githubusercontent.com/2114189/222967780-8af8981a-e69d-4304-9dc4-0235582fa4f4.png) ### After ![image](https://user-images.githubusercontent.com/2114189/222967575-c21c23d4-0200-4e09-aac3-57895e853000.png) ![image](https://user-images.githubusercontent.com/2114189/222967585-8b8da262-bc96-441a-9851-8d3845f2659d.png) ![image](https://user-images.githubusercontent.com/2114189/222967595-58d9bea5-6df4-41fa-bf8a-86704117959d.png) ![image](https://user-images.githubusercontent.com/2114189/222967608-38757c1a-b8bd-4ebf-b7a8-3b30edb7f303.png) ![image](https://user-images.githubusercontent.com/2114189/222967623-9849a339-6fae-4484-8fa5-939e2fdacbf5.png) ![image](https://user-images.githubusercontent.com/2114189/222967633-4383d7dd-62ba-47a3-8c10-86f7ca7757ae.png) --------- Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
Replace go-gitea#23310, Close go-gitea#19733 And fix various UI problems, including regressions from go-gitea#22959 go-gitea#22950 and more. ## SVG Detection The old regexp may mismatch non-SVG files. This PR adds new tests for those cases. ## UI Changes ### Before ![image](https://user-images.githubusercontent.com/2114189/222967716-f6ad8721-f46a-4a3f-9eb0-a89e488d3436.png) ![image](https://user-images.githubusercontent.com/2114189/222967780-8af8981a-e69d-4304-9dc4-0235582fa4f4.png) ### After ![image](https://user-images.githubusercontent.com/2114189/222967575-c21c23d4-0200-4e09-aac3-57895e853000.png) ![image](https://user-images.githubusercontent.com/2114189/222967585-8b8da262-bc96-441a-9851-8d3845f2659d.png) ![image](https://user-images.githubusercontent.com/2114189/222967595-58d9bea5-6df4-41fa-bf8a-86704117959d.png) ![image](https://user-images.githubusercontent.com/2114189/222967608-38757c1a-b8bd-4ebf-b7a8-3b30edb7f303.png) ![image](https://user-images.githubusercontent.com/2114189/222967623-9849a339-6fae-4484-8fa5-939e2fdacbf5.png) ![image](https://user-images.githubusercontent.com/2114189/222967633-4383d7dd-62ba-47a3-8c10-86f7ca7757ae.png) --------- Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
Backport #23312 Replace #23310, Close #19733 And fix various UI problems, including regressions from #22959 #22950 and more. ## SVG Detection The old regexp may mismatch non-SVG files. This PR adds new tests for those cases. ## UI Changes ### Before ![image](https://user-images.githubusercontent.com/2114189/222967716-f6ad8721-f46a-4a3f-9eb0-a89e488d3436.png) ![image](https://user-images.githubusercontent.com/2114189/222967780-8af8981a-e69d-4304-9dc4-0235582fa4f4.png) ### After ![image](https://user-images.githubusercontent.com/2114189/222967575-c21c23d4-0200-4e09-aac3-57895e853000.png) ![image](https://user-images.githubusercontent.com/2114189/222967585-8b8da262-bc96-441a-9851-8d3845f2659d.png) ![image](https://user-images.githubusercontent.com/2114189/222967595-58d9bea5-6df4-41fa-bf8a-86704117959d.png) ![image](https://user-images.githubusercontent.com/2114189/222967608-38757c1a-b8bd-4ebf-b7a8-3b30edb7f303.png) ![image](https://user-images.githubusercontent.com/2114189/222967623-9849a339-6fae-4484-8fa5-939e2fdacbf5.png) ![image](https://user-images.githubusercontent.com/2114189/222967633-4383d7dd-62ba-47a3-8c10-86f7ca7757ae.png) Co-authored-by: wxiaoguang <wxiaoguang@gmail.com> Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
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
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
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).
Close #22847
This PR:
showElem
and related functionsFrom now on:
do not use:
only use:
cc: @silverwind , this is the all-in-one PR