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

Use vue instead of plain template to render suscribe div of issue #28914

Closed
wants to merge 8 commits into from

Conversation

lunny
Copy link
Member

@lunny lunny commented Jan 24, 2024

This PR did two things.

  • Introducing initComponent which can make the component initlize simple than before
  • Introducing issue/Suscribe.vue to instead of old template method

This can be compared with #28908 about a different framework to do the same thing.

Screen Recording 2024-01-24 at 19 05 54 2024-01-24 19_08_26

@lunny lunny added topic/ui Change the appearance of the Gitea UI type/refactoring Existing code has been cleaned up. There should be no new functionality. labels Jan 24, 2024
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jan 24, 2024
@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 24, 2024
Copy link
Contributor

@wxiaoguang wxiaoguang left a comment

Choose a reason for hiding this comment

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

I think "partially page reloading" (eg: htmx or jQuery.load or some Gitea's frontend mechanisms) is more suitable for this case, because overall Gitea is using backend template rendering heavily.

Using Vue for small components would make some things more complex:

  1. Page flickering (because the Vue components are rendered after the page is loaded/rendered), some people dislike such flickering, then to fix such flickering it requires more hacky code.
  2. Additional work for maintaining status/locale, additional JavaScript code.

Personally I prefer to fine tune existing mechanisms like link-action / form-fetch-action, while since most people like htmx and there would be some standards, so I think htmx could also do well.

@denyskon
Copy link
Member

Isn't for example the loading indicator missing?

Overall, I can mostly agree with @wxiaoguang - we should not mix go templates and vue heavily, it creates confusion, makes refactoring harder, adds additional code which can be avoided usind htmx and so on.

I could imagine full Vue rewrites being beneficial for some components that are designed as dynamic in the first place, but for such small elements htmx looks like a better solution.

@lunny
Copy link
Member Author

lunny commented Jan 29, 2024

Isn't for example the loading indicator missing?

Overall, I can mostly agree with @wxiaoguang - we should not mix go templates and vue heavily, it creates confusion, makes refactoring harder, adds additional code which can be avoided usind htmx and so on.

I could imagine full Vue rewrites being beneficial for some components that are designed as dynamic in the first place, but for such small elements htmx looks like a better solution.

The loading added, but the real problem is the div flickering @wxiaoguang mentioned. An obvious layout reaggrange can be found in the area of the subscribe button. The external div of the button needs a fixed height if we want to avoid that.

@lunny
Copy link
Member Author

lunny commented Jan 31, 2024

Close as htmx version merged.

@lunny lunny closed this Jan 31, 2024
@lunny lunny deleted the lunny/subscribe_async branch January 31, 2024 04:20
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 4, 2024
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. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. topic/ui Change the appearance of the Gitea UI type/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants