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.

Sign up for GitHub

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

Jump to bottom

Fix strange UI behavior of cancelling dismiss review modal #25133

Merged
merged 2 commits into from
Jun 8, 2023
Merged

Fix strange UI behavior of cancelling dismiss review modal #25133

merged 2 commits into from
Jun 8, 2023

Conversation

yp05327
Copy link
Contributor

@yp05327 yp05327 commented Jun 8, 2023 •
edited
Loading

Fixes #25130

The old code uses $(this).next() to get dismiss-review-modal.
At first, it will get $(#dismiss-review-modal), but the next time it will get $(#dismiss-review-modal).next();
and then $(#dismiss-review-modal).next().next();.
Because div dismiss-review-modal will be removed when dismiss-review-btn clicked.

Maybe the right usage is adding show-modal class and data-modal attribute.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jun 8, 2023
@pull-request-size pull-request-size bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jun 8, 2023
@yp05327 yp05327 force-pushed the fix-dissmiss-review-modal branch from 869d690 to b8e4920 Compare June 8, 2023 06:03
@pull-request-size pull-request-size bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jun 8, 2023
@yp05327 yp05327 added the topic/ui Change the appearance of the Gitea UI label Jun 8, 2023
@lunny lunny added type/bug backport/v1.20 This PR should be backported to Gitea 1.20 labels Jun 8, 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 Jun 8, 2023
@lunny lunny requested a review from wolfogre June 8, 2023 08:06
wolfogre
wolfogre approved these changes Jun 8, 2023
View reviewed changes
Copy link
Member

@wolfogre wolfogre left a comment

Choose a reason for hiding this comment

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

Confirmed as fixed.

@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 Jun 8, 2023
@silverwind silverwind enabled auto-merge (squash) June 8, 2023 08:21
@silverwind silverwind added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Jun 8, 2023
@lunny lunny added this to the 1.21.0 milestone Jun 8, 2023
@silverwind silverwind merged commit b5a2bb9 into go-gitea:main Jun 8, 2023
GiteaBot pushed a commit to GiteaBot/gitea that referenced this pull request Jun 8, 2023
@yp05327 @GiteaBot
Fix strange UI behavior of cancelling dismiss review modal (go-gitea#…
be1bd97 
…25133)

Fixes go-gitea#25130

The old code uses `$(this).next()` to get `dismiss-review-modal`.
At first, it will get `$(#dismiss-review-modal)`, but the next time it
will get `$(#dismiss-review-modal).next();`
and then `$(#dismiss-review-modal).next().next();`.
Because div `dismiss-review-modal` will be removed when
`dismiss-review-btn` clicked.

Maybe the right usage is adding `show-modal` class and `data-modal`
attribute.
@GiteaBot GiteaBot mentioned this pull request Jun 8, 2023
Closed
@GiteaBot GiteaBot added backport/done All backports for this PR have been created and removed reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. labels Jun 8, 2023
wxiaoguang
wxiaoguang reviewed Jun 8, 2023
View reviewed changes
templates/repo/issue/view_content/sidebar.tmpl
{{svg "octicon-x" 20}}
</a>
<div class="ui small modal" id="dismiss-review-modal">
<div class="ui small modal dismiss-review-modal" id="dismiss-review-modal">
Copy link
Contributor

Choose a reason for hiding this comment

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

This PR is wrong.

You are using the same id="dismiss-review-modal" in a range loop

Please revert or fix ASAP.

Copy link
Member

Choose a reason for hiding this comment

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

The id is not introduced by this PR so we can send a new PR to fix it.

Copy link
Contributor

Choose a reason for hiding this comment

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

This ID was never used.

But this PR uses it by data-modal="#dismiss-review-modal".

Then which modal does it refer to?

Copy link
Member

Choose a reason for hiding this comment

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

The problem is inside range there will be multiple iterations but HTML ids must be unique in the whole page. I think the solution is to move the modal outside the range as there can effectively only be one visible at a time.

Copy link
Contributor

Choose a reason for hiding this comment

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

As a quick fix, I think it can be combined with a ID, like #dismiss-review-modal-{{.Review.ID}}

Copy link
Member

@silverwind silverwind Jun 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, seems like a good temporary solution. The modal depends on .Review.ID so can not be easily be moved outside the loop as-is.

silverwind added a commit to silverwind/gitea that referenced this pull request Jun 8, 2023
@silverwind
Merge branch 'main' into buttonfix
757ca22 
* main:
  Modify OAuth login ui and fix display name, iconurl related logic (go-gitea#25030)
  Fix open redirect check for more cases (go-gitea#25143)
  Update js dependencies (go-gitea#25137)
  Remove duplicated functions when deleting a branch (go-gitea#25128)
  Add codeowners feature (go-gitea#24910)
  Fix strange UI behavior of cancelling dismiss review modal (go-gitea#25133)
  Fix `MilestoneIDs` when querying issues (go-gitea#25125)
  Fix incorrect git ignore rule and add missing license files (go-gitea#25135)
  Change branch name from master to main in some documents' links (go-gitea#25126)
  Remove incorrect element ID on "post-install" page (go-gitea#25104)
  [skip ci] Updated translations via Crowdin
  Improve notification icon and navbar  (go-gitea#25111)
  fix swagger documentation for multiple files API endpoint (go-gitea#25110)
@yp05327 yp05327 mentioned this pull request Jun 9, 2023
Merged
zjjhot added a commit to zjjhot/gitea that referenced this pull request Jun 9, 2023
@zjjhot
Merge remote-tracking branch 'upstream/main'
482bdb7 
* upstream/main:
  [skip ci] Updated translations via Crowdin
  Modify OAuth login ui and fix display name, iconurl related logic (go-gitea#25030)
  Fix open redirect check for more cases (go-gitea#25143)
  Update js dependencies (go-gitea#25137)
  Remove duplicated functions when deleting a branch (go-gitea#25128)
  Add codeowners feature (go-gitea#24910)
  Fix strange UI behavior of cancelling dismiss review modal (go-gitea#25133)
  Fix `MilestoneIDs` when querying issues (go-gitea#25125)
  Fix incorrect git ignore rule and add missing license files (go-gitea#25135)
  Change branch name from master to main in some documents' links (go-gitea#25126)
  Remove incorrect element ID on "post-install" page (go-gitea#25104)
  [skip ci] Updated translations via Crowdin
  Improve notification icon and navbar  (go-gitea#25111)
  fix swagger documentation for multiple files API endpoint (go-gitea#25110)
  Fix webauthn regression and improve code (go-gitea#25113)
  Add details summary for vertical menus in settings to allow toggling (go-gitea#25098)
  Fix 500 error caused by notifications without an issue such as repo transfers (go-gitea#25101)
silverwind added a commit that referenced this pull request Jun 9, 2023
@yp05327 @GiteaBot @silverwind
Fix #25133 (#25162)
81211db 
Fix #25133

Thanks @wxiaoguang @silverwind.
I'm sorry I made a mistake, it will be fixed in this PR.

---------

Co-authored-by: Giteabot <[email protected]>
Co-authored-by: silverwind <[email protected]>
silverwind pushed a commit to silverwind/gitea that referenced this pull request Jun 9, 2023
@yp05327 @silverwind
Fix strange UI behavior of cancelling dismiss review modal (go-gitea#…
d30aae5 
…25133)

Fixes go-gitea#25130

The old code uses `$(this).next()` to get `dismiss-review-modal`.
At first, it will get `$(#dismiss-review-modal)`, but the next time it
will get `$(#dismiss-review-modal).next();`
and then `$(#dismiss-review-modal).next().next();`.
Because div `dismiss-review-modal` will be removed when
`dismiss-review-btn` clicked.

Maybe the right usage is adding `show-modal` class and `data-modal`
attribute.
silverwind added a commit to silverwind/gitea that referenced this pull request Jun 9, 2023
@yp05327 @GiteaBot @silverwind
Fix go-gitea#25133 (go-gitea#25162)
40a5348 
Fix go-gitea#25133

Thanks @wxiaoguang @silverwind.
I'm sorry I made a mistake, it will be fixed in this PR.

---------

Co-authored-by: Giteabot <[email protected]>
Co-authored-by: silverwind <[email protected]>
@silverwind silverwind mentioned this pull request Jun 9, 2023
Merged
lunny pushed a commit that referenced this pull request Jun 11, 2023
@silverwind
Fix strange UI behavior of cancelling dismiss review modal (#25172)
224ee0d 
Backport clean cherry-picks of
#25133 and
#25162 to 1.20.
zjjhot added a commit to zjjhot/gitea that referenced this pull request Jun 12, 2023
@zjjhot
Merge remote-tracking branch 'upstream/main'
1fa6c06 
* upstream/main:
  [skip ci] Updated licenses and gitignores
  Add `WithPullRequest` for `actionsNotifier` (go-gitea#25144)
  [skip ci] Updated translations via Crowdin
  [skip ci] Updated translations via Crowdin
  Update github.com/google/go-github to v53 (go-gitea#25157)
  Fix bug for code search if code is disabled (go-gitea#25173)
  Minor arc-green color tweaks (go-gitea#25175)
  Fix duplicate Reviewed-by trailers (go-gitea#24796)
  Fix go-gitea#25133 (go-gitea#25162)
  Fix mobile navbar and misc cleanups (go-gitea#25134)
  Button and color enhancements (go-gitea#24989)
  Fix setup-go actions (go-gitea#25167)

# Conflicts:
#	templates/base/head_navbar.tmpl
@yp05327 yp05327 deleted the fix-dissmiss-review-modal branch June 15, 2023 00:41
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Sep 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@wxiaoguang wxiaoguang wxiaoguang left review comments

@silverwind silverwind silverwind approved these changes

@lunny lunny lunny left review comments

@wolfogre wolfogre wolfogre approved these changes

Assignees
No one assigned
Labels
backport/done All backports for this PR have been created backport/v1.20 This PR should be backported to Gitea 1.20 lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. topic/ui Change the appearance of the Gitea UI type/bug
Projects
None yet
Milestone
1.21.0
Development

Successfully merging this pull request may close these issues.

Cancelling dismissing a review could cause strange UI behavior
6 participants
@yp05327 @lunny @silverwind @wxiaoguang @wolfogre @GiteaBot