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

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

Open
wants to merge 33 commits into
base: main
Choose a base branch
from
Open

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

wants to merge 33 commits into from

Conversation

lunny
Copy link
Member

@lunny lunny commented Mar 29, 2024 •
edited

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 •
edited

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
@yp05327
Copy link
Contributor

yp05327 commented Apr 18, 2024

image
template:
image
image
css:
image
js:
image

@silverwind
Copy link
Member

I can check the frontend parts.

@lunny
Copy link
Member Author

lunny commented Apr 18, 2024

I can check the frontend parts.

Thank you. And I think we should rename board -> column but still keep board view. Because we will have table view in future like what GH did.

@lunny lunny mentioned this pull request Apr 19, 2024
Open
5 tasks
@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 •
edited

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.

@lunny lunny mentioned this pull request Apr 25, 2024
Closed
@yp05327
Copy link
Contributor

yp05327 commented Apr 30, 2024

https://github.com/go-gitea/gitea/pull/30170/files#diff-f4279417070a8e33829c338abeb42877500377f490abb1495ae6357d50b6a765R46

plz check all locale
It has been changed:
image

@yp05327 yp05327 mentioned this pull request Apr 30, 2024
Merged
7 tasks
yp05327
yp05327 approved these changes Apr 30, 2024
View reviewed changes
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@silverwind silverwind silverwind approved these changes

@yp05327 yp05327 yp05327 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. 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
Milestone
1.23.0
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@lunny @denyskon @silverwind @wxiaoguang @yp05327 @GiteaBot