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

[sourcery] Choose Emojis, as a browser-rendered block #14612

Closed
wants to merge 18 commits into from
Closed

[sourcery] Choose Emojis, as a browser-rendered block #14612

wants to merge 18 commits into from

Conversation

c4lliope
Copy link
Contributor

@c4lliope c4lliope commented Feb 8, 2021

Original: please see https://gitea.com/sourcery/sourcery/pulls/1
Please also see #14581 as background.

My code here replaces a golang .tmpl files using a React component, or a "block".
Necessary information for each component is placed in a JSON blob inside the .tmpl file,
and is read by index.js to build the block.

This is a drop-in replacement for the Emoji add_reaction template,
and paves our road for making similar changes in:

  • templates/repo/issue/view_content/add_reaction.tmpl
  • templates/repo/issue/view_content/reactions.tmpl
  • templates/repo/issue/view_content/context_menu.tmpl
  • templates/repo/issue/view_content/attachments.tmpl

I am opening this issue in case you would like to keep these changes inside mainline Gitea.
Perhaps, you can also deny my changes and I shall keep going inside https://gitea.com/sourcery/sourcery.
Small merges are easier, so if you like my goal maybe we can keep our codebases aligned.
I hope my ideas make your grade.

Cheers and 干杯, @lunny and squad. 🚢

@jolheiser
Copy link
Member

jolheiser commented Feb 8, 2021

I don't know if I'm comfortable adding React to our stack. We already use a weird mix of vanilla, jQuery, and some Vue. (among others)

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 8, 2021
@6543 6543 modified the milestone: 1.15.0 Feb 8, 2021
@6543
Copy link
Member

6543 commented Feb 8, 2021

I'm also not comfortable with this. ... I think there is an Issue with this discussion if we should have a one-page app or not and the consensus was. That dont do that. Instead enhance the api so others can make it as an own project ... we would then present it as alternative fronted to the docs ...

@c4lliope
Copy link
Contributor Author

c4lliope commented Feb 8, 2021

https://github.com/c4lliope/gitea/commit/7d8a225557d4e4b86ccdcbf6b78244e41506a458
handles templates/repo/issue/view_content/context_menu.tmpl

@techknowlogick
Copy link
Member

Putting on project owner hat, and first want to thank you for your PR. However, we won't be merging this, as @jolheiser has said, we have already many frameworks in our stack, and elsewhere we have previously stated we won't be moving towards including react in the project for those reasons as well as as a project we need to maintain these additions of which as a project we are more familiar with Vue. The great thing about SPAs is that you can build one without changing the project at all. In fact, there is one public project that has already build react components for interacting with Gitea (on mobile now, so I can't find link).

@c4lliope
Copy link
Contributor Author

c4lliope commented Feb 8, 2021

@jolheiser and @6543, I'm glad you responded.

I am hoping by changing .tmpl files to React-based blocks, our code can:

As I described in #14581, I imagine these goals are enough reason to make a branch of the codebase. I'm only curious how aligned you'd like to keep them.

@6543
Copy link
Member

6543 commented Feb 8, 2021

I'm aware that the api is not jet feature complete ... I like to address this in the next release ...

For API's the goal is to be backwards compatible as long as it doesn't affect security ... so If you program an interface for gitea's api you can assume it will stay as is for the upcoming releases - if not consider it as a bug.

Proposals for needed API's are welcome!

@go-gitea go-gitea locked and limited conversation to collaborators Mar 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants