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

feat: Add search bar on user profile page. #787

Merged
merged 1 commit into from
Feb 4, 2017

Conversation

appleboy
Copy link
Member

@appleboy appleboy commented Jan 29, 2017

Screenshot as below:

screen shot 2017-01-29 at 8 57 31 pm

  • Sort and search keyword on repository list

@lunny lunny added this to the 1.1.0 milestone Jan 29, 2017
@lunny lunny added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Jan 29, 2017
@lunny
Copy link
Member

lunny commented Jan 31, 2017

LGTM

@tboerger tboerger added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Jan 31, 2017
@lunny
Copy link
Member

lunny commented Feb 2, 2017

conflicted

@appleboy
Copy link
Member Author

appleboy commented Feb 3, 2017

@lunny Done.

}

ctx.Data["Repos"] = repos
ctx.Data["Page"] = paginater.New(int(count), setting.UI.User.RepoPagingNum, page, 5)
Copy link
Member

Choose a reason for hiding this comment

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

What is the hard-coded 5 here ?

Copy link
Member

Choose a reason for hiding this comment

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

$ go doc github.com/Unknwon/paginater.New
func New(total, pagingNum, current, numPages int) *Paginater
    New initialize a new pagination calculation and returns a Paginater as
    result.

What's the meaning of numPages ? How's that different from total/pagingNum ?
@unknwon ? Maybe the documentation on that could be improved

Copy link
Member

Choose a reason for hiding this comment

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

From the README.md of the paginater:

        // Arguments:
        // - Total number of rows
        // - Number of rows in one page
        // - Current page number 
        // - Number of page links
        p := paginater.New(45, 10, 3, 3)

I still don't understand what Number of page links means though.
Is it the expected number of pages ? As that could be computed as ceil(Total number of rows / Number of rows in one page), right ?

Copy link
Member

Choose a reason for hiding this comment

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

Ok I think I got it: it's the number of page links you want to be visible in a template. Sounds a bit wrong to mix output control and flow control to me, but this is not the place to discuss this. Still maybe it's good to have a configuration in settings about the number of pager links ? As I see that hard-coded 5 in many places even only in this file...

Copy link
Member Author

Choose a reason for hiding this comment

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

@strk I think this may be a bug. I change the Number of page links to 2. It still show five items in a template.

Copy link
Member

@strk strk left a comment

Choose a reason for hiding this comment

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

Sounds good to me, beside the desire of see the hard-coded "number of page links" pager parameter be replaced by a system configuration.

@strk
Copy link
Member

strk commented Feb 4, 2017

LGTM, even :)

@tboerger tboerger 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 Feb 4, 2017
@lunny
Copy link
Member

lunny commented Feb 4, 2017

let L-G-T-M work

@lunny lunny merged commit a90a215 into go-gitea:master Feb 4, 2017
@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/done This PR has enough approvals to get merged. There are no important open reservations anymore. 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.

4 participants