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

Decouple the issue-template code from comment_tab.tmpl #23556

Merged
merged 3 commits into from Mar 21, 2023
Merged

Decouple the issue-template code from comment_tab.tmpl #23556

merged 3 commits into from Mar 21, 2023

Conversation

wxiaoguang
Copy link
Contributor

It would help #23290

The issue-template code is only useful for "new issue" or "new PR", so it could only be put in the new_form.tmpl

@wolfogre how do you think?

delvh
delvh reviewed Mar 18, 2023
View reviewed changes
Copy link
Member

@delvh delvh left a comment •
edited

Choose a reason for hiding this comment

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

Logic seems to be the same.
Apart from the comment below, I see neither something that would hold this PR back, nor how that would be helpful for the issue you linked

templates/repo/issue/comment_tab.tmpl
@@ -28,7 +12,6 @@
{{.locale.Tr "loading"}}
</div>
</div>
{{end}}
{{if .IsAttachmentEnabled}}
Copy link
Member

Choose a reason for hiding this comment

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

Alternatively, we could also extract the lines below into its own subtemplate, so that we don't need to declare them twice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My plan is introducing a "ComboEditor" for Gitea in the future, a ComboEditor could have: markdown toolbar, preview, textarea, dropzone(uploader).

So leave it to future.

@GiteaBot GiteaBot added lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Mar 18, 2023
@delvh delvh added this to the 1.20.0 milestone Mar 18, 2023
@delvh delvh added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Mar 18, 2023
@wxiaoguang
Copy link
Contributor Author

wxiaoguang commented Mar 19, 2023 •
edited

nor how that would be helpful for the issue you linked

It would make introducing the new editor easier. Otherwise if a lot of logics are mixed together, it's difficult to get clear refactoring. In my plan, the first step is making the "comment_tab.tmpl" use the new "ComboEditor", the "issue template" part is totally unrelated.

And one more thing, the "issue template" part doesn't match the name "comment_tab.tmpl". comment_tab means that it's used for writing user comments, and provide a tab to switch between editor/previewer.

wolfogre
wolfogre approved these changes Mar 21, 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.

Since it follows the same logic, I would say yes if it could benefit another PR.

@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 Mar 21, 2023
@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 Mar 21, 2023
@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Mar 21, 2023
@jolheiser jolheiser enabled auto-merge (squash) March 21, 2023 20:36
@jolheiser
Copy link
Member

🎺 🤖

@jolheiser jolheiser merged commit 76a1edf into go-gitea:main Mar 21, 2023
2 checks passed
@jolheiser jolheiser removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Mar 21, 2023
@wxiaoguang wxiaoguang deleted the decouple-issue-template branch March 22, 2023 01:45
zjjhot added a commit to zjjhot/gitea that referenced this pull request Mar 22, 2023
@zjjhot
Merge remote-tracking branch 'upstream/main'
7095008 
* upstream/main:
  Use a general approch to improve a11y for all checkboxes and dropdowns. (go-gitea#23542)
  [skip ci] Updated translations via Crowdin
  Update PR documentation (go-gitea#23620)
  Set opaque background on markup and images (go-gitea#23578)
  Decouple the issue-template code from comment_tab.tmpl  (go-gitea#23556)
  Remove `id="comment-form"` dead code, fix tag (go-gitea#23555)
  Introduce path Clean/Join helper functions (go-gitea#23495)
  Remove conflicting CSS rules on notifications, improve notifications table (go-gitea#23565)
  Remove @metalmatze as maintainer (go-gitea#23612)
  Keep (add if not existing) xmlns attribute for generated SVG images (go-gitea#23410)
@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.
Reviewers

@lunny lunny lunny approved these changes

@wolfogre wolfogre wolfogre approved these changes

@delvh delvh delvh approved these changes

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
Milestone
1.20.0
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@wxiaoguang @jolheiser @lunny @wolfogre @delvh @GiteaBot