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

[RFC] Added data-pagination to view output. #92

Closed
wants to merge 1 commit into from
Closed

[RFC] Added data-pagination to view output. #92

wants to merge 1 commit into from

Conversation

leevigraham
Copy link
Contributor

The data attribute is a json_encoded array of the following:

  • url
  • totalResults
  • totalPages
  • currentPage
  • currentPageOffsetStart
  • currentPageOffsetEnd

Builds on top off PR #91

Is this something that interests anyone?

This update makes it easier to add other JS hooks / events. I'm currently using it to build a perPage select box:

image

The method in my widget looks like:

        _processPerPage: function (event) {

            event.preventDefault();

            var paginationData = this.element.find(this.options.paginationSelector).data('pagination'),
                currentOffset = paginationData.currentPageOffsetStart,
                perPage = $(event.target).val(),
                newPage = Math.ceil(currentOffset / perPage),
                uri = new URI(paginationData.url);

            uri.setSearch(this.options.pageKey, newPage);
            uri.setSearch(this.options.perPageKey, perPage);

            $.ajax({
                type: 'get',
                url: uri.href(),
                beforeSend: $.proxy(this._beforeSend, this),
                success: $.proxy(this._success, this),
                complete: $.proxy(this._complete, this)
            });

        },

The data attribute is a json_encoded array of the following:

* url
* totalResults
* totalPages
* currentPage
* currentPageOffsetStart
* currentPageOffsetEnd

This update makes it easier to add other JS hooks / events.
@pablodip
Copy link
Contributor

This should be optional, thus the views work the same as before by default. Also tests should be passing (some of the views are not passing now), and you should add tests for this as well.


private function getPaginationData(){
return array(
"url" => $this->template->generateRoute(null),
Copy link
Contributor

Choose a reason for hiding this comment

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

You can access to the route generator in the view directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm. How? I can see it anywhere?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm probably daft but I don't understand. Are you suggesting that the router should be saved as a class variable in the DefaultView class?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, using somehow the routegenerator that is already in the view.

@pablodip
Copy link
Contributor

Do you plan to finish this?

@leevigraham
Copy link
Contributor Author

I do. I was justa bit confused.

To clarify you're suggesting:

When the RouteGenerator is passed into configureTemplate() set a class variable on DefaultView?

@pablodip
Copy link
Contributor

Or just in the render method, as configurateTemplate what does is configurating the template)

@@ -23,14 +23,15 @@ class DefaultTemplate extends Template
'css_dots_class' => 'dots',
'css_current_class' => 'current',
'dots_text' => '...',
'container_template' => '<nav>%pages%</nav>',
Copy link
Contributor

Choose a reason for hiding this comment

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

Why removing the option?

@pablodip
Copy link
Contributor

Could you rebase and add documentation for this please?

@pablodip
Copy link
Contributor

pablodip commented Nov 2, 2013

Do you plan to do this?

@pablodip
Copy link
Contributor

pablodip commented Jan 4, 2014

Are you alive? :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants