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

Branches management UI #2464

Closed
wants to merge 5 commits into from
Closed

Conversation

lunny
Copy link
Member

@lunny lunny commented Sep 5, 2017

As title. This should be the first step to move commits, branches, tags as a submenu of Code.

@lunny lunny added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Sep 5, 2017
@lunny lunny added this to the 1.3.0 milestone Sep 5, 2017
@@ -579,6 +579,14 @@ func RegisterRoutes(m *macaron.Macaron) {
m.Get("/milestones", repo.Milestones)
}, context.RepoRef())

m.Group("/branches", func() {
Copy link
Member

Choose a reason for hiding this comment

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

I think this group needs context.CheckUnit(models.UnitTypeCode)

Copy link
Member Author

Choose a reason for hiding this comment

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

done.


brs, err := ctx.Repo.GitRepo.GetBranches()
func loadBranches(c *context.Context) []*Branch {
Copy link
Member

Choose a reason for hiding this comment

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

Could we name it ctx for consistency with other handlers? Likewise in other places

m.Get("/all", repo.AllBranches)
m.Post("/delete/*", reqSignIn, reqRepoWriter, repo.DeleteBranchPost)
}, repo.MustBeNotBare, func(c *context.Context) {
c.Data["PageIsViewFiles"] = true
Copy link
Member

Choose a reason for hiding this comment

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

Is "PageIsViewFiles" used anywhere? I can't find it anywhere in this PR or in existing code.


protectBranches, err := c.Repo.Repository.GetProtectedBranches()
if err != nil {
c.Handle(500, "GetProtectBranchesByRepoID", err)
Copy link
Member

Choose a reason for hiding this comment

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

Wrong name

@lunny
Copy link
Member Author

lunny commented Sep 5, 2017

@ethantkoenig all done.

@tboerger tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Sep 5, 2017
@ethantkoenig
Copy link
Member

Would be nice to provide screenshots, otherwise LGTM

@tboerger tboerger 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 Sep 11, 2017
@Morlinest
Copy link
Member

I am for adding some example screenshots too.

@lunny
Copy link
Member Author

lunny commented Sep 11, 2017

Just found a bug
untitled

@lunny
Copy link
Member Author

lunny commented Sep 15, 2017

seems this is duplicated with #2108

@lafriks
Copy link
Member

lafriks commented Sep 15, 2017

Yes these PR need to be merged in one :)

@lunny lunny mentioned this pull request Sep 21, 2017
8 tasks
@lunny lunny removed this from the 1.3.0 milestone Sep 21, 2017
@lunny
Copy link
Member Author

lunny commented Sep 21, 2017

#2108 will replace this one, so I close here.

@lunny lunny closed this Sep 21, 2017
@lunny lunny deleted the lunny/branches_status_bar branch October 26, 2017 15:19
@go-gitea go-gitea locked and limited conversation to collaborators Nov 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 1 This PR needs approval from one additional maintainer to be merged. type/feature Completely new functionality. Can only be merged if feature freeze is not active.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants