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

Change ID pattern of raw content container for issue #21966

Merged
merged 3 commits into from
Dec 9, 2022

Conversation

fsologureng
Copy link
Contributor

@fsologureng fsologureng commented Nov 29, 2022

Implement differentiation to html id for issue raw content container.

Fixes #21965

@codecov-commenter
Copy link

Codecov Report

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

@@           Coverage Diff           @@
##             main   #21966   +/-   ##
=======================================
  Coverage        ?   48.14%           
=======================================
  Files           ?     1037           
  Lines           ?   141357           
  Branches        ?        0           
=======================================
  Hits            ?    68052           
  Misses          ?    65165           
  Partials        ?     8140           

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

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Nov 29, 2022
@ghost ghost added the topic/ui Change the appearance of the Gitea UI label Nov 29, 2022
@silverwind
Copy link
Member

Something is wrong here, you seem to have commited a .patch file instead of editing the actual file.

@fsologureng
Copy link
Contributor Author

Something is wrong here, you seem to have commited a .patch file instead of editing the actual file.

Excuse me. You are right. I think it's ready now.

@fsologureng
Copy link
Contributor Author

Thanks for reviewing @silverwind.

@silverwind
Copy link
Member

Are you sure this won't break any comment-* links?

@fsologureng
Copy link
Contributor Author

Are you sure this won't break any comment-* links?

Ok. I finally understand how things work for quotes. I hope the fix is correct this time.
Thank you very much for your time.

@techknowlogick techknowlogick changed the title Change ID pattern of raw content container for issue. Fixes #21965. Change ID pattern of raw content container for issue Dec 1, 2022
@zeripath
Copy link
Contributor

zeripath commented Dec 3, 2022

So the problem here is that people may have written links in their comments looking to link to commit-$COMMENT_ID and these will now break.

Is it really necessary to add the raw- prefix and instead only change the link to the issue message to issue-$ID instead?

@silverwind
Copy link
Member

I don't understand the original issue but I think if we change all the comment- ids, it'd break all existing links to comments, so this seems like a definitive no-go to me. We must maintain compatibility.

@fsologureng
Copy link
Contributor Author

fsologureng commented Dec 5, 2022

@zeripath: So the problem here is that people may have written links in their comments looking to link to commit-$COMMENT_ID and these will now break.

@silverwind: I think if we change all the comment- ids, it'd break all existing links to comments, so this seems like a definitive no-go to me. We must maintain compatibility.

The proposed ID change is for the raw content inside issues and comments (required for quoting before doing post), not for the issue ID itself nor the comment ID itself. Those IDs (for linking) remain intact in the templates in the data-reference attribute.

@fsologureng
Copy link
Contributor Author

I don't understand the original issue

The problem is a collision between html IDs of raw-content of an issue and a single one of its comments, because their current html ID prefix (comment-) is the same for both. So, taking into account the fact that is possible that equals internal ID (one from the issue and the other from any of its comments) appears in the same page, the unwanted behaviour is that quoting to reply or quoting for a new issue with a reference, takes a quoted text that belongs to the other object (issue or comment, respectively).

@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 Dec 7, 2022
@fsologureng
Copy link
Contributor Author

I updated the commit:

  • Adding 3 more template points. There are 5 types of comment.
  • Redefining the new ID following suffix notation (-raw) of other auxiliar IDs (-write, -preview) of comments.
  • Changing javascript quoting functions making them agnostics about data-target value structure which is now self-contained.

@@ -54,7 +54,7 @@
<span class="no-content">{{$.root.locale.Tr "repo.issues.no_content"}}</span>
{{end}}
</div>
<div id="comment-{{.ID}}" class="raw-content hide">{{.Content}}</div>
<div id="issuecomment-{{.ID}}-raw" class="raw-content hide">{{.Content}}</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

Could .HashTag be used here instead of .ID?

The problem of using "issuecomment-{{.ID}}-raw" here is: it introduces inconsistency, future developers won't understand easily that data-target="{{.item.HashTag}}-raw" means something like "issuecomment-{{.ID}}-raw".

Copy link
Contributor Author

@fsologureng fsologureng Dec 9, 2022

Choose a reason for hiding this comment

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

It could be, but the line immediately following has a data-write="issuecomment-{{.ID}}-write" attribute (which I think will also leave an inconsistency) and, on the other hand, the data-target="{{.item.HashTag}}-raw" attribute means either "issuecomment-{.ID}}-raw" or "issue-{.ID}}-raw" depending on the context.

Update: Maybe another PR could change all the "issuecomment-{.ID}}-suffix" as "{{.HashTag}}-suffix" of the template for consistency.

Copy link
Contributor

@wxiaoguang wxiaoguang Dec 9, 2022

Choose a reason for hiding this comment

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

Hmm yup, it was messy already. 😂 It could LGTM.

@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 Dec 9, 2022
@wxiaoguang
Copy link
Contributor

wxiaoguang commented Dec 9, 2022

I don't understand the original issue but I think if we change all the comment- ids, it'd break all existing links to comments, so this seems like a definitive no-go to me. We must maintain compatibility.

These IDs are for "hidden" texts (to be quoted later), it won't break anything IMO.

@wxiaoguang wxiaoguang merged commit 097d4e3 into go-gitea:main Dec 9, 2022
@fsologureng fsologureng deleted the fix-first-issue-id branch December 9, 2022 16:56
zjjhot added a commit to zjjhot/gitea that referenced this pull request Dec 12, 2022
* giteaofficial/main:
  Use multi reader instead to concat strings (go-gitea#22099)
  Fix sorting admin user list by last login (go-gitea#22081)
  Fix wrong default value for update checker on app.example.ini (go-gitea#22084)
  fix(config): remove context on config template (go-gitea#22096)
  [skip ci] Updated licenses and gitignores
  Update xorm (go-gitea#22094)
  Remove unnecessary whitespace in snapcraft.yaml (go-gitea#22090)
  Rename almost all Ctx functions (go-gitea#22071)
  Change ID pattern of raw content container for issue (go-gitea#21966)
  Optimize html templates (go-gitea#22080)
  Add API management for issue/pull and comment attachments (go-gitea#21783)
  Rename actions to operations on UI (go-gitea#22067)
  Update go dev dependencies (go-gitea#22064)
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
This pull request was closed.
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
Projects
None yet
6 participants