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

Rename project board -> column to make the UI less confusing #30170

Merged
merged 35 commits into from
May 27, 2024

Conversation

lunny
Copy link
Member

@lunny lunny commented Mar 29, 2024

This PR split the Board into two parts. One is the struct has been renamed to Column and the second we have a Template Type.

But to make it easier to review, this PR will not change the database schemas, they are just renames. The database schema changes could be in future PRs.

@lunny lunny added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Mar 29, 2024
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 29, 2024
@pull-request-size pull-request-size bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Mar 29, 2024
@github-actions github-actions bot added modifies/go Pull requests that update Go code modifies/migrations labels Mar 29, 2024
@denyskon
Copy link
Member

You stole my PR idea!!! 🤣 Will review later today. This has to be done carefully to avoid breaking templates for example.

@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 29, 2024
@silverwind
Copy link
Member

Do we need any renames on the templates and UI parts?

@silverwind silverwind self-requested a review March 29, 2024 10:06
@GiteaBot GiteaBot added lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Mar 29, 2024
@denyskon
Copy link
Member

IIRC, the router also used Boards, so it should also be present in the template. Same for corresponding JS code. I can't check currently, but I will later today

@silverwind
Copy link
Member

Definitly should do it all at once, not just backend only.

@wxiaoguang
Copy link
Contributor

I would suggest do not merge it until 1.22.1 or 1.22.2, otherwise any bug fix would be difficult to backport

@silverwind
Copy link
Member

silverwind commented Mar 29, 2024

I would suggest do not merge it until 1.22.1 or 1.22.2, otherwise any bug fix would be difficult to backport

I disagree. Now is the best time for big refactors. And if those refactors are well-tested, they should be backported as well to enable future backports to be automatic.

@wxiaoguang
Copy link
Contributor

And if those refactors are well-tested

The problem is, I didn't see how "those refactors are well-tested"

@lunny lunny marked this pull request as draft March 29, 2024 17:14
@github-actions github-actions bot added the modifies/templates This PR modifies the template files label Mar 30, 2024
@lunny lunny marked this pull request as ready for review March 30, 2024 14:16
@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 30, 2024
@lunny
Copy link
Member Author

lunny commented Apr 20, 2024

Looks like most frontend board usages are right.

@silverwind
Copy link
Member

silverwind commented Apr 21, 2024

data-id="{{.ID}}" resolves to incorrect board_3. I don't think it's in use in frontend, so maybe it could be changed to just a number.

@silverwind
Copy link
Member

JS and CSS look good to me. There is simplication possible like removing the board class but I will not do that in this PR.

@yp05327
Copy link
Contributor

yp05327 commented Apr 30, 2024

Copy link
Contributor

@yp05327 yp05327 left a comment

Choose a reason for hiding this comment

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

I suggest merging this unless we got at least 3 approves

@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 Apr 30, 2024
@wxiaoguang
Copy link
Contributor

Quote my comments above:

I would suggest do not merge it until 1.22.1 or 1.22.2, otherwise any bug fix would be difficult to backport

At least, wait for 1.22.0 to be stable.

And if those refactors are well-tested

The problem is, I didn't see how "those refactors are well-tested"

How to prove it is well-tested?

@lunny
Copy link
Member Author

lunny commented Apr 30, 2024

Quote my comments above:

I would suggest do not merge it until 1.22.1 or 1.22.2, otherwise any bug fix would be difficult to backport

At least, wait for 1.22.0 to be stable.

And if those refactors are well-tested

The problem is, I didn't see how "those refactors are well-tested"

How to prove it is well-tested?

This is just a rename refactor, it will not change the previous logic. So I think the previous tests will still work if they exist. I don't know how should we add tests for the renaming behavior.

@wxiaoguang
Copy link
Contributor

This is just a rename refactor, it will not change the previous logic. So I think the previous tests will still work if they exist. I don't know how should we add tests for the renaming behavior.

It doesn't seem to be as simple as a renaming refactor. There are many "names" passed by string, no strict code check, and I guess there were some conflicts resolved manually.

So at least, I think every changed route handler and page should be tested manually?

@techknowlogick techknowlogick changed the title Rename project board -> column to make the UI less confusion Rename project board -> column to make the UI less confusing May 12, 2024
@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label May 27, 2024
@lunny lunny enabled auto-merge (squash) May 27, 2024 07:58
@lunny lunny merged commit 9875110 into go-gitea:main May 27, 2024
26 checks passed
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label May 27, 2024
@lunny lunny deleted the lunny/rename_board_column branch May 27, 2024 12:18
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Aug 25, 2024
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. modifies/docs modifies/go Pull requests that update Go code modifies/migrations modifies/templates This PR modifies the template files modifies/translation size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. topic/projects 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.

6 participants