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

Allow commit status popup on /pulls page #19507

Merged
merged 5 commits into from Apr 26, 2022

Conversation

parnic
Copy link
Contributor

@parnic parnic commented Apr 26, 2022

The /pulls page doesn't contain a "repository" element, so the early-out here was preventing the commit status popup hook from working. However, the only thing the .repository element is being used for here is determining whether the popup should be on the right or on the left, so we don't actually need the element to exist for the hook to work.

The /pulls page doesn't contain a "repository" element, so the early-out here was preventing the commit status popup hook from working. However, the only thing the .repository element is being used for here is determining whether the popup should be on the right or on the left, so we don't actually need the element to exist for the hook to work.

Pull request go-gitea#19375 allows the statuses on /pulls pages to appear clickable, but this commit is required to make the popup actually work there.
@parnic
Copy link
Contributor Author

parnic commented Apr 26, 2022

Pull request #19375 allows the statuses on /pulls pages to appear clickable, but this commit is required to make the popup actually work there, so consider this an amendment to that PR (I would have just added this there, but that PR is already approved and waiting on being merged, so I didn't want to muddy the waters).

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 26, 2022
@wxiaoguang
Copy link
Contributor

Which template? If the page (template) is repository related, it should have the CSS class repository.

Otherwise the commit-statuses-trigger related code could be extracted into a new dedicated function, instead of mixing them together.

@parnic
Copy link
Contributor Author

parnic commented Apr 26, 2022

Which template? If the page (template) is repository related, it should have the CSS class repository.

Otherwise the commit-statuses-trigger related code could be extracted into a new dedicated function, instead of mixing them together.

This originates from routers\web\user\home.go's func Pulls(ctx *context.Context) which ultimately ends up rendering user/dashboard/issues. For the Try.Gitea page, that would be https://try.gitea.io/pulls and the org/team-filtered-org version of the same page. No element on this page uses the repository class, but commit statuses can still show up there.

I'm happy to move that function somewhere else if that's more desirable. Does it need a new .js file or where would you prefer it to live?

@wxiaoguang
Copy link
Contributor

I see, I feel that it could be clearer to move the commit-statuses-trigger into a new function.

More details: at the moment, Gitea's JS code are using a simple mechanism to describe the dependency:

function initXxx() {
    if (!$('.xxx').length) return;
}

So if some unrelated code appears before the if (!$('.xxx').length) return;, it would make the dependency unclear and harder to get refactored in future.

@parnic
Copy link
Contributor Author

parnic commented Apr 26, 2022

I see, I feel that it could be clearer to move the commit-statuses-trigger into a new function.

More details: at the moment, Gitea's JS code are using a simple mechanism to describe the dependency:

function initXxx() {
    if (!$('.xxx').length) return;
}

So if some unrelated code appears before the if (!$('.xxx').length) return;, it would make the dependency unclear and harder to get refactored in future.

Understood - that makes sense. I'll make that change.

@parnic
Copy link
Contributor Author

parnic commented Apr 26, 2022

@wxiaoguang pushed those changes.

@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 Apr 26, 2022
@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 Apr 26, 2022
@6543 6543 added type/bug topic/ui Change the appearance of the Gitea UI labels Apr 26, 2022
@6543 6543 added this to the 1.17.0 milestone Apr 26, 2022
@6543
Copy link
Member

6543 commented Apr 26, 2022

🪨

@6543 6543 merged commit 2347c9e into go-gitea:main Apr 26, 2022
@parnic parnic deleted the allow-commit-status-popup-on-pulls-page branch April 26, 2022 23:42
zjjhot added a commit to zjjhot/gitea that referenced this pull request Apr 28, 2022
* giteaofficial/main: (21 commits)
  Prevent intermittent race in attribute reader close (go-gitea#19537)
  Make repository file list useable on mobile (go-gitea#19515)
  Update image URL for Discord webhook (go-gitea#19536)
  [skip ci] Updated translations via Crowdin
  Fix 64-bit atomic operations on 32-bit machines (go-gitea#19531)
  Fix `upgrade.sh` script error with `su -c` (go-gitea#19483)
  When view _Siderbar or _Footer, just display once (go-gitea#19501)
  Fix migrate release from github (go-gitea#19510)
  Prevent dangling archiver goroutine (go-gitea#19516)
  Don't let repo clone URL overflow (go-gitea#19517)
  Add commit status popup to issuelist (go-gitea#19375)
  Disable unnecessary GitHooks elements (go-gitea#18485)
  Disable unnecessary GitHooks elements
  Improve dashboard's repo list performance (go-gitea#18963)
  By default force vertical tabs on mobile (go-gitea#19486)
  Refactor readme file renderer (go-gitea#19502)
  Allow package dump skipping (go-gitea#19506)
  Unset git author/committer variables when running integration tests (go-gitea#19512)
  Allow commit status popup on /pulls page (go-gitea#19507)
  Use router param for filepath in GetRawFile (go-gitea#19499)
  ...
parnic-sks pushed a commit to parnic-sks/gitea that referenced this pull request May 16, 2022
* Allow commit status popup on /pulls page

The /pulls page doesn't contain a "repository" element, so the early-out here was preventing the commit status popup hook from working. However, the only thing the .repository element is being used for here is determining whether the popup should be on the right or on the left, so we don't actually need the element to exist for the hook to work.

Pull request go-gitea#19375 allows the statuses on /pulls pages to appear clickable, but this commit is required to make the popup actually work there.

* Move commit statuses popup hook to dedicated func

* Add missing import
AbdulrhmnGhanem pushed a commit to kitspace/gitea that referenced this pull request Aug 24, 2022
* Allow commit status popup on /pulls page

The /pulls page doesn't contain a "repository" element, so the early-out here was preventing the commit status popup hook from working. However, the only thing the .repository element is being used for here is determining whether the popup should be on the right or on the left, so we don't actually need the element to exist for the hook to work.

Pull request go-gitea#19375 allows the statuses on /pulls pages to appear clickable, but this commit is required to make the popup actually work there.

* Move commit statuses popup hook to dedicated func

* Add missing import
@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/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants