Extend jQuery with customized show/hide/toggle #22884

wxiaoguang · 2023-02-13T06:19:45Z

Follow

Move helpers to be prefixed with gt- #22879

This PR makes jQuery works well in all cases, it is the first step for

[Proposal] Refactor frontend hide/hidden mechanism #22847
In the end, there will be only one hiding-method instead of the 5 old different methods, and there won't be any conflict.

About jQuery

jQuery's show/hide/toggle doesn't work with our gt-hidden class which has !important

jQuery can not show a hidden element with .gt-hidden { display: none !important; } because of the !important.
The !important is necessary for many cases. eg: overwrite the other display: flex

Ref: jQuery's show/hide: https://github.com/jquery/jquery/blob/main/src/css/showHide.js

Some more info:

The most hacky part (.hide.show-outdate and .hide.hide-outdated) has been cleaned up
The next PR could just do a search&replace like c6c3afc to remove the legacy .hide class.
Next PR will have more details (comments & documents) about the hiding-problem and the hiding-method.

Screenshot (the refactored show/hide buttons work well):

The `show` button

Switch to the `hide` button and show details

codecov-commenter · 2023-02-13T07:16:23Z

Codecov Report

❗ No coverage uploaded for pull request base (main@b6d7722). Click here to learn what that means.
The diff coverage is n/a.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@           Coverage Diff           @@
##             main   #22884   +/-   ##
=======================================
  Coverage        ?   47.32%           
=======================================
  Files           ?     1113           
  Lines           ?   150188           
  Branches        ?        0           
=======================================
  Hits            ?    71076           
  Misses          ?    70668           
  Partials        ?     8444

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

wxiaoguang · 2023-02-13T18:42:05Z

web_src/less/_base.less

-
-  &.hide-outdated {
-    display: none !important;
-  }


The most hacky part has been removed.

wxiaoguang · 2023-02-13T18:42:18Z

web_src/less/_review.less

@@ -57,7 +57,6 @@
 .show-outdated,
 .hide-outdated {
  &:extend(.unselectable);
-  display: block !important;


Another hacky part, it is also removed.

silverwind · 2023-02-14T18:19:17Z

Assuming we want to eventually get rid of jQuery, I would oppose to extending it.

Instead I would use simple helper functions that works with both plain Node/NodeList and optionally jQuery collections (thought ideally we would just do $el[0] at the call site, eliminating the need for the last else branch):

function show(els) {
  if (els instanceof Node) {
    els.classList.remove('gt-hidden');
  } else if (els instanceof NodeList) {
    for (const el of els) {
      els.classList.remove('gt-hidden');
    }
  } else if (els instanceof $) {
    els.each(el, function() {
      this.classList.remove("gt-hidden");
    });
  }
}

function hide(els) {
  if (els instanceof Node) {
    els.classList.add('gt-hidden');
  } else if (els instanceof NodeList) {
    for (const el of els) {
      els.classList.add('gt-hidden');
    }
  } else if (els instanceof $) {
    els.each(el, function() {
      this.classList.add("gt-hidden");
    });
  }
}

This will allow enabling the jquery/no-hide and jquery/no-show eslint rules which is a good step on the eventual complete removal of jQuery.

web_src/js/jquery.js

wxiaoguang · 2023-02-15T09:31:50Z

Assuming we want to eventually get rid of jQuery, I would oppose to extending it.

This is the first step, to avoid breaking anything. You can fine tune anything after the code gets refactored correctly.

silverwind · 2023-02-15T10:46:37Z

What do you think about the show/hide functions above? I think they are supremely simple and universally usable. They don't rely on inline styles so work with strict CSP as well.

wxiaoguang · 2023-02-15T12:22:27Z

What do you think about the show/hide functions above? I think they are supremely simple and universally usable. They don't rely on inline styles so work with strict CSP as well.

They do not work with .popup { display: none; ... } <div class="popup">, as a refactroing, keep the old logic as much as possible.
I do not think el.style.display violate CSP [Proposal] Refactor frontend hide/hidden mechanism #22847 (comment) :
- I think unsafe-inline only forbids the style="xxx" in HTML, but it doesn't forbid changing style by JS, right? Correct me if I am wrong.

wxiaoguang · 2023-02-15T12:51:23Z

If you want to skip this step (customize the jQuery's show/hide), then the next PR must do all the following things at the same time:

Replace all .hide to .gt-hidden
Replace all $.show()/$.hide() to the new show()/hide()

In my mind, "Replace all jQuery.show()/jQuery.hide() to the new show()/hide()" is the biggest change, it will change a lot of lines/logic and it may need more time. So this PR just makes jQuery works with .gt-hidden, then there will be enough time to do more show()/hide() refactoring.

With this step, all code then can start using .gt-hidden (even if they are using jQuery.show/hide)

wxiaoguang · 2023-02-15T13:04:36Z

@silverwind if you still prefer to not touch the jQuery, the separate PR is

First step to refactor the .hide to .gt-hidden #22916

silverwind · 2023-02-15T18:01:10Z

They do not work with .popup { display: none; ... }

I'd argue these should be refactored to have the hidden class initially in HTML.

I do not think el.style.display violate CSP

It does appear writing to .style.<prop> is exempt from CSP as per MDN, so it should be fine indeed. Still I prefer a universal class approach than messing directly with inline display.

wxiaoguang · 2023-02-16T01:27:30Z

They do not work with .popup { display: none; ... }

I'd argue these should be refactored to have the hidden class initially in HTML.

Of course, you could argue.

If you have checked every CSS style to guarantee that there is no such usage anymore, then you do not need that getComputedStyle

BUT, if you haven't checked, to make sure nothing would be broken, you absolutely need that getComputedStyle to work with .popup { display: none; ... }.

Could you check everything and give me a feed back?

Update: you could take a look at #22916 first, it only contains the .hide changes, no show()/hide() trick.

First step to refactor the .hide to .gt-hidden #22916

I do not think el.style.display violate CSP

It does appear writing to .style.<prop> is exempt from CSP as per MDN, so it should be fine indeed. Still I prefer a universal class approach than messing directly with inline display.

I know you prefer. I have explained my plain in #22847 , the "Solution": Drop all other classes/attributes, only use the .gt-hidden to hide

[Proposal] Refactor frontend hide/hidden mechanism #22847

A separate PR from #22884 （without touching the jQuery methods)

wxiaoguang · 2023-02-17T05:58:57Z

Replaced by #22950

fix

309d8bf

wxiaoguang force-pushed the fix-jquery-hide branch from 7e62981 to 309d8bf Compare February 13, 2023 06:39

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 13, 2023

wxiaoguang added 2 commits February 14, 2023 02:01

Merge branch 'main' into fix-jquery-hide

ce2f29b

refactor .hide class

c6c3afc

wxiaoguang mentioned this pull request Feb 13, 2023

[Proposal] Refactor frontend hide/hidden mechanism #22847

Closed

wxiaoguang commented Feb 13, 2023

View reviewed changes

Merge branch 'main' into fix-jquery-hide

d2557d5

silverwind reviewed Feb 14, 2023

View reviewed changes

web_src/js/jquery.js Show resolved Hide resolved

add comment

12ffb0d

wxiaoguang mentioned this pull request Feb 15, 2023

First step to refactor the .hide to .gt-hidden #22916

Merged

lunny pushed a commit that referenced this pull request Feb 16, 2023

First step to refactor the .hide to .gt-hidden (#22916)

2e97a77

A separate PR from #22884 （without touching the jQuery methods)

wxiaoguang closed this Feb 17, 2023

wxiaoguang deleted the fix-jquery-hide branch February 17, 2023 06:21

go-gitea locked and limited conversation to collaborators May 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend jQuery with customized show/hide/toggle #22884

Extend jQuery with customized show/hide/toggle #22884

wxiaoguang commented Feb 13, 2023 •

edited

Loading

codecov-commenter commented Feb 13, 2023

wxiaoguang Feb 13, 2023

wxiaoguang Feb 13, 2023

silverwind commented Feb 14, 2023 •

edited

Loading

wxiaoguang commented Feb 15, 2023

silverwind commented Feb 15, 2023

wxiaoguang commented Feb 15, 2023 •

edited

Loading

wxiaoguang commented Feb 15, 2023 •

edited

Loading

wxiaoguang commented Feb 15, 2023

silverwind commented Feb 15, 2023 •

edited

Loading

wxiaoguang commented Feb 16, 2023 •

edited

Loading

wxiaoguang commented Feb 17, 2023

Extend jQuery with customized show/hide/toggle #22884

Extend jQuery with customized show/hide/toggle #22884

Conversation

wxiaoguang commented Feb 13, 2023 • edited Loading

About jQuery

Some more info:

Screenshot (the refactored show/hide buttons work well):

The show button

Switch to the hide button and show details

codecov-commenter commented Feb 13, 2023

Codecov Report

wxiaoguang Feb 13, 2023

Choose a reason for hiding this comment

wxiaoguang Feb 13, 2023

Choose a reason for hiding this comment

silverwind commented Feb 14, 2023 • edited Loading

wxiaoguang commented Feb 15, 2023

silverwind commented Feb 15, 2023

wxiaoguang commented Feb 15, 2023 • edited Loading

wxiaoguang commented Feb 15, 2023 • edited Loading

wxiaoguang commented Feb 15, 2023

silverwind commented Feb 15, 2023 • edited Loading

wxiaoguang commented Feb 16, 2023 • edited Loading

wxiaoguang commented Feb 17, 2023

wxiaoguang commented Feb 13, 2023 •

edited

Loading

The `show` button

Switch to the `hide` button and show details

silverwind commented Feb 14, 2023 •

edited

Loading

wxiaoguang commented Feb 15, 2023 •

edited

Loading

wxiaoguang commented Feb 15, 2023 •

edited

Loading

silverwind commented Feb 15, 2023 •

edited

Loading

wxiaoguang commented Feb 16, 2023 •

edited

Loading