Remove assertion debug code for show/hide refactoring #23576

wxiaoguang · 2023-03-19T15:56:54Z

When doing the refactoring:

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

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

Fix some more hidden problems #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

silverwind · 2023-03-19T19:10:00Z

I guess these few weeks since it was added was already enough. Glad to see it go 😉

wxiaoguang · 2023-03-20T02:06:56Z

I wouldn't assume it's "enough" at the moment. 1.19 hasn't been released, I am not sure that whether users would report related "show/hide" bugs.

silverwind · 2023-03-20T15:23:43Z

Well then let's skip the backport and this will get more than enough testing through that :)

wxiaoguang · 2023-04-01T04:06:01Z

Haven't seen related reports for a while, let's clean the assertion code and get this in 1.19.1?

delvh · 2023-04-01T12:35:08Z

web_src/js/utils/dom.js

  if (force === true) {
    el.classList.remove('gt-hidden');
-    assertShown(el, true);
  } else if (force === false) {
    el.classList.add('gt-hidden');
-    assertShown(el, false);
  } else if (force === undefined) {
-    const wasShown = window.config.runModeIsProd ? undefined : isShown(el);
    el.classList.toggle('gt-hidden');


Can't that be simplified now to el.classList.toggle('gt-hidden', force)?

It probably can but I guess it may be slower then explit add/remove. But would need to check how browsers implement toggle.

Probably does not matter though, as the difference ought to be tiny.

I think maybe one day we should make it work with official hidden attribute together, because the [hidden] is standard.

Then we need to remove (in show) and check (in toggle) the hidden attribute. Then such if blocks would help.

codecov-commenter · 2023-04-01T14:15:16Z

Codecov Report

Merging #23576 (4d1ad21) into main (f521e88) will decrease coverage by 0.16%.
The diff coverage is 28.82%.

❗ Current head 4d1ad21 differs from pull request most recent head 4a00a25. Consider uploading reports for the commit 4a00a25 to get more accurate results

@@            Coverage Diff             @@
##             main   #23576      +/-   ##
==========================================
- Coverage   47.14%   46.98%   -0.16%     
==========================================
  Files        1149     1158       +9     
  Lines      151446   153187    +1741     
==========================================
+ Hits        71397    71982     +585     
- Misses      71611    72716    +1105     
- Partials     8438     8489      +51

Impacted Files	Coverage Δ
cmd/dump.go	`0.66% <0.00%> (-0.01%)`	⬇️
cmd/mailer.go	`0.00% <0.00%> (ø)`
cmd/manager.go	`0.00% <0.00%> (ø)`
cmd/manager_logging.go	`0.00% <0.00%> (ø)`
cmd/migrate_storage.go	`5.76% <0.00%> (-0.12%)`	⬇️
cmd/restore_repo.go	`0.00% <0.00%> (ø)`
cmd/web.go	`0.00% <0.00%> (ø)`
models/actions/run.go	`1.63% <0.00%> (-0.10%)`	⬇️
models/actions/runner.go	`1.44% <ø> (ø)`
models/packages/package.go	`45.45% <0.00%> (-1.13%)`	⬇️
... and 66 more

... and 72 files with indirect coverage changes

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

techknowlogick · 2023-04-02T03:39:07Z

🤖 🎺

@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

@wxiaoguang

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).

* upstream/main: [skip ci] Updated translations via Crowdin Update JS deps (go-gitea#23853) Added close/open button to details page of milestone (go-gitea#23877) Check `IsActionsToken` for LFS authentication (go-gitea#23841) Prefill input values in oauth settings as intended (go-gitea#23829) Display image size for multiarch container images (go-gitea#23821) Use clippie module to copy to clipboard (go-gitea#23801) Remove assertion debug code for show/hide refactoring (go-gitea#23576) [skip ci] Updated translations via Crowdin Remove jQuery ready usage (go-gitea#23858) Fix JS error when changing PR's target branch (go-gitea#23862) Improve action log display with control chars (go-gitea#23820) Fix review conversation reply (go-gitea#23846) Improve home page template, fix Sort dropdown menu flash (go-gitea#23856) Make first section on home page full width (go-gitea#23854) [skip ci] Updated translations via Crowdin Fix incorrect CORS failure detection logic (go-gitea#23844)

fix

0eb10b1

wxiaoguang mentioned this pull request Mar 19, 2023

Use a general approach to show tooltip, fix temporary tooltip bug #23574

Merged

wxiaoguang marked this pull request as draft March 19, 2023 16:48

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

silverwind approved these changes Mar 20, 2023

View reviewed changes

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 Mar 20, 2023

wxiaoguang marked this pull request as ready for review April 1, 2023 04:04

silverwind added the outdated/backport/v1.19 This PR should be backported to Gitea 1.19 label Apr 1, 2023

lunny approved these changes Apr 1, 2023

View reviewed changes

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 Apr 1, 2023

delvh reviewed Apr 1, 2023

View reviewed changes

delvh approved these changes Apr 1, 2023

View reviewed changes

lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Apr 1, 2023

lunny added 2 commits April 1, 2023 21:01

Merge branch 'main' into remove-showhide-assertion

4d1ad21

Merge branch 'main' into remove-showhide-assertion

14eeb71

techknowlogick added this to the 1.20.0 milestone Apr 2, 2023

techknowlogick enabled auto-merge (squash) April 2, 2023 03:38

Merge branch 'main' into remove-showhide-assertion

4a00a25

techknowlogick merged commit fcb9ef8 into go-gitea:main Apr 2, 2023

GiteaBot mentioned this pull request Apr 2, 2023

Remove assertion debug code for show/hide refactoring (#23576) #23868

Merged

GiteaBot added the backport/done All backports for this PR have been created label Apr 2, 2023

wxiaoguang deleted the remove-showhide-assertion branch April 2, 2023 08:17

lunny removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Apr 2, 2023

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove assertion debug code for show/hide refactoring #23576

Remove assertion debug code for show/hide refactoring #23576

wxiaoguang commented Mar 19, 2023 •

edited

Loading

silverwind commented Mar 19, 2023

wxiaoguang commented Mar 20, 2023

silverwind commented Mar 20, 2023

wxiaoguang commented Apr 1, 2023

delvh Apr 1, 2023

silverwind Apr 1, 2023 •

edited

Loading

wxiaoguang Apr 1, 2023

codecov-commenter commented Apr 1, 2023 •

edited

Loading

techknowlogick commented Apr 2, 2023

Remove assertion debug code for show/hide refactoring #23576

Remove assertion debug code for show/hide refactoring #23576

Conversation

wxiaoguang commented Mar 19, 2023 • edited Loading

silverwind commented Mar 19, 2023

wxiaoguang commented Mar 20, 2023

silverwind commented Mar 20, 2023

wxiaoguang commented Apr 1, 2023

delvh Apr 1, 2023

Choose a reason for hiding this comment

silverwind Apr 1, 2023 • edited Loading

Choose a reason for hiding this comment

wxiaoguang Apr 1, 2023

Choose a reason for hiding this comment

codecov-commenter commented Apr 1, 2023 • edited Loading

Codecov Report

techknowlogick commented Apr 2, 2023

wxiaoguang commented Mar 19, 2023 •

edited

Loading

silverwind Apr 1, 2023 •

edited

Loading

codecov-commenter commented Apr 1, 2023 •

edited

Loading