Skip to content

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.

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