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

A minimal change to replace data calls with attr as per guidelines #19900

Merged
merged 3 commits into from
Jun 6, 2022

Conversation

Ryuno-Ki
Copy link
Contributor

@Ryuno-Ki Ryuno-Ki commented Jun 5, 2022

Tiny step towards #18345 (comment)

This affects the manage topics on a repository. Namely the done
button once changes are made.

<a class="ui button primary" href="javascript:;" id="save_topic"
data-link="{{.RepoLink}}/topics">{{.i18n.Tr "repo.topic.done"}}</a>

I'm unhappy with the associated template, as there is an anchor
element for something that should be a button. Since it must not be
mixed, I haven't refactored the code into a Vue component yet.

How are refactorings organised in this project?

Signed-off-by: André Jaenisch [email protected]

This affects the manage topics on a repository. Namely the done
button once changes are made.

I'm unhappy with the associated template, as there is an anchor
element for something that should be a button. Since it must not be
mixed, I haven't refactored the code into a Vue component yet.

Signed-off-by: André Jaenisch <[email protected]>
@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Jun 6, 2022
@techknowlogick techknowlogick added type/refactoring Existing code has been cleaned up. There should be no new functionality. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jun 6, 2022
@GiteaBot GiteaBot added the lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. label Jun 6, 2022
@wxiaoguang wxiaoguang merged commit ebeb6e7 into go-gitea:main Jun 6, 2022
@wxiaoguang
Copy link
Contributor

wxiaoguang commented Jun 6, 2022

Since it must not be mixed, I haven't refactored the code into a Vue component yet.

In my mind, there might be some options for rewriting the code into Vue:

  1. All components from scratch
    • then there should be a well-designed Vue-based UI component library first
  2. Re-use the Fomantic UI CSS styles, write template and JS in Vue
    • it seems the easiest way
  3. Mix Fomantic UI jQuery and Vue JS
    • the problem is that Fomantic UI is a MVC framework and Vue is a MVVM framework, they don't work in the same way, it would made the mixed code very difficult to be maintained or refactored in future.

And still in my mind:

  • I prefer option 1 but it requires a lot of design & plan work, the work hasn't been started yet, so not feasible at the moment.
  • option 2 is used mostly in my code and existing code.
  • although option 3 is discouraged, if it must be used and the code is clear and fine tuned, it should also work .....

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Jun 6, 2022

About the mixed code, you can have a look at repo-search:

https://github.com/go-gitea/gitea/blob/main/web_src/js/components/DashboardRepoList.js
https://github.com/go-gitea/gitea/blob/main/templates/user/dashboard/repolist.tmpl

I do not think it's a good practice to use Vue.


The development guidelines are always open (actually, most of them were written recently), if you have ideas about how to make the code base more healthy and maintainable, please suggest 🙏

@wxiaoguang
Copy link
Contributor

Please help to review #19901

@Ryuno-Ki Ryuno-Ki deleted the repo-home-data branch June 6, 2022 10:49
@Ryuno-Ki
Copy link
Contributor Author

Ryuno-Ki commented Jun 6, 2022

I feel like this should be another issue (since discussion tab is disabled in this repo), but allow me to reply here for the time being:

In my mind, there might be some options for rewriting the code into Vue:

1. All components from scratch
   
   * then there should be a well-designed Vue-based UI component library first

https://storybook.js.org/ is becoming industry standard. How do you deal with disabled JavaScript in these cases? I've read elsewhere, that Gitea wants to be able to cater for these cases (and I assume, you don't want to run Server-Side Rendering).

2. Re-use the Fomantic UI CSS styles, write template and JS in Vue
   
   * it seems the easiest way

I would suggest this for now, too. With the option to extract LESS files from Fomantic once the rewrite is complete (to drop the dependency entirely).

3. Mix Fomantic UI jQuery and Vue JS
   
   * the problem is that Fomantic UI is a MVC  framework and Vue is a MVVM framework, they don't work in the same way, it would made the mixed code very difficult to be maintained or refactored in future.

I agree. It will be a lot of work to detangle. Perhaps some meta-issue could be tracked with a task list listing all files and PRs that reference their refactor. As I mentioned in that chat, the sheer number of open PRs make me nervous that I would duplicate work.

And still in my mind:

* I prefer option 1 but it requires a lot of design & plan work, the work hasn't been started yet, so not feasible at the moment.

* option 2 is used mostly in my code and existing code.

* although option 3 is discouraged, if it must be used and the code is clear and fine tuned, it should also work .....

I would suggest going with option 2 until the Fomantic dependency can be dropped. Looking at fomantic/Fomantic-UI#319 (comment) there is a transition planned to TypeScript and SASS (SASS is more popular then LESS, TypeScript has a learning curve).

If option 3 is applied, perhaps open an issue to follow-up with a refactoring (or note that in the meta issue).

I came across certain elements, that would make a good fit for a component.
If we spend the time to configure Storybook, then components could be defined there and once „approved” introduced into the codebase. Be aware, that npm audit has several known issues with Storybook.
Let me know how to proceed here.

(I'm okay if you copy these conversations into a new issue and refernce this PR there)

zjjhot added a commit to zjjhot/gitea that referenced this pull request Jun 6, 2022
* giteaofficial/main:
  Move some repository related code into sub package (go-gitea#19711)
  A minimal change to replace data calls with attr as per guidelines (go-gitea#19900)
  Modernize JS build scripts (go-gitea#19824)
  [skip ci] Updated translations via Crowdin
  Update MAINTAINERS (go-gitea#19896)
@wxiaoguang
Copy link
Contributor

There are discussions (as old as back to 2019)

The Gitea UI problem is really a long story .....

AbdulrhmnGhanem pushed a commit to kitspace/gitea that referenced this pull request Aug 24, 2022
…o-gitea#19900)

This affects the manage topics on a repository.
Namely the done button once changes are made.

Signed-off-by: André Jaenisch <[email protected]>

Co-authored-by: Lunny Xiao <[email protected]>
Co-authored-by: wxiaoguang <[email protected]>
@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. 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.

5 participants