[added] Pagination component #786

roadmanfong · 2015-06-04T13:23:32Z

I tried to make pagination component, and I'm not sure what should I put in unit test.

Closes #22

mtscout6 · 2015-06-04T14:43:43Z

Thanks for putting this together.

Unit tests should ensure:

classNames are applied or not applied as appropriate for the various states
Mouse and keyboard usage (ReactTestUtils makes this feasible.)
- ie. ensure that onSelect event is fired when you expect.
Html layout is as expected in the various states, this can be done in the same tests that test classNames (For example when you use the ellipsis prop the li for the ... button is added.)
Accessibility concerns are tested. Even if they are just static having tests to ensure they are there are good to ensure they are not accidentally removed.

You can look at the other tests in this library for inspiration there's a lot of good examples on how to test components there.

mtscout6 · 2015-06-04T14:54:46Z

src/Pagination.js

+          className={pagenumber === this.props.activePage ? 'active' : null}
+          data-pagenumber={pagenumber}
+          onClick={this._onSelect}>
+          <a href='#' onClick={this.noClick}>{pagenumber}</a>


The anchor fills the entire <li> block. Can you just put the _onSelect on the anchor and forego one on the li? I don't see a need for noClick if you did.

Also, we have a new SafeAnchor that would work well here: https://github.com/react-bootstrap/react-bootstrap/blob/v0.24-rc/src/SafeAnchor.js. Though this PR would need to be re-opened against the v0.24.0-rc branch as the SafeAnchor is not slated to hit till v0.24.0.

This also raises another question I have about supplying an href or href template for each page, I can see some value in being able to do so. This implementation is assuming a purely single page app approach.

mtscout6 · 2015-06-04T18:51:20Z

src/Pagination.js

+
+  propTypes: {
+    activePage: React.PropTypes.number,
+    items: React.PropTypes.number,


Is it possible for this to be an array of links?

Or possibly include a prop for hrefTemplate were the page number can be inserted. Thus allowing this to work with non-SPA sites.

I'm thinking something like:

React.PropTypes.oneOfType([ React.PropTypes.number, React.PropTypes.arrayOf(React.PropTypes.string) ]);

mtscout6 · 2015-06-04T19:08:54Z

Can you add a link in the side nav for Pagination, right under Pager

mtscout6 · 2015-06-04T19:11:15Z

I created #788 to so we don't forget to do the SafeAnchor change for the v0.24 release in this component.

mtscout6 · 2015-06-04T19:20:58Z

test/PaginationSpec.js

+    );
+    let pageButtons = ReactTestUtils.scryRenderedDOMComponentsWithTag(instance, 'li');
+    assert.equal(pageButtons.length, 5);
+    assert.equal(pageButtons[2].getDOMNode().getAttribute('class').match(/active/)[0], 'active');


React.findDOMNode is more future proof than getDOMNode() We still have a number of tests using it but if we can use the newer function that would help us out.

Also this assertion is cleaner with React.findDOMNode(pageButtons[2]).className.should.match(/\bactive\b/) The \b in the regex is a word boundary marker so if we need to add more classes in the future this assertion would still pass.

AlexKVal · 2015-06-05T19:31:43Z

[fixed] should be placed into comment's header only if you're fixing something in the existing codebase.

In this case you are just correcting the code you are proposing, so you don't need it.

I know it is all very confusing at the beginning 🍒
Let's it not stop you on your way ❇️
I was doing such kind of errors with all those commits by myself many times 😄

roadmanfong · 2015-06-05T19:33:12Z

Oh I see, should I rebase the commit?

AlexKVal · 2015-06-05T19:36:20Z

You can do git push -f as many times as you need 👍
Until you like the PR by yourself 😉

AlexKVal · 2015-06-05T19:38:49Z

Anyway you will have to remove this [fixed] before this PR can be merged,
because this project uses automatic changelog generation from these [_tag_] tags in commit headers.
By this package https://github.com/mtscout6/mt-changelog

roadmanfong · 2015-06-05T19:47:19Z

Ok I removed it.

A lot of things to learn.
Thank you for your patience. 😃

AlexKVal · 2015-06-05T19:51:17Z

No worries.
It is thank you for contributing ! 🍒

A lot of things to learn.

Open Source - it means we all always will learn 🌈 😄

mtscout6 · 2015-06-05T20:21:02Z

src/Pagination.js

+    if(this.props.onSelect){
+      this.props.onSelect(event, selectedEvent);
+    }
+  },


If you are not doing anything in here why not just pass this.props.onSelect right on through to the PaginationButton without this intermediary? You are already doing the conditional check for presence within the PaginationButton.

roadmanfong · 2015-06-06T10:08:45Z

Is it good to squash all my commits now?

AlexKVal · 2015-06-06T10:35:46Z

docs/src/ComponentsPage.js

+                  <p>Provide pagination links for your site or app with the multi-page pagination component.</p>
+                  <ReactPlayground codeText={Samples.PaginationBasic} />
+
+                  <p>More controller such as first, last, previous, next and ellipsis.</p>


controller => controllers ? or handlers ? or buttons ?
@mtscout6 What would you suggest ?

I would say options

AlexKVal · 2015-06-06T10:42:13Z

Before git push run linter check by npm run lint.
There should be no any warnings. ❇️

Such these:

src/PaginationButton.js
  15:19  warning  'onSelect' is missing in props validation for PaginationButton  react/prop-types
  16:57  warning  'eventKey' is missing in props validation for PaginationButton  react/prop-types
  17:17  warning  'onSelect' is missing in props validation for PaginationButton  react/prop-types

mtscout6 · 2015-06-06T15:50:13Z

If you can just add that test assertion for the ellipse and the wording in the docs then go ahead and squash. The concern I brought up with items and anchor hrefs can follow up in a different PR if someone needs it.

roadmanfong · 2015-06-07T03:26:23Z

Ok, I added test assert for the ellipse, and fixed some wording in the docs. Please have a look of last commit.

roadmanfong · 2015-06-08T01:23:10Z

active and disabled now are props of PaginationButton.

Rewrote this lines

let hiddenPagesBefore = activePage - parseInt(maxButtons / 2);
startPage = hiddenPagesBefore > 1 ? hiddenPagesBefore : 1;
hasHiddenPagesAfter = startPage + maxButtons <= items;

Also have some changes in wording of document

 <p>Provide pagination links for your site or app with the multi-page pagination component. Set <code>items</code> to the number of pages. <code>activePage</code> prop dictates which page is active</p>
 <ReactPlayground codeText={Samples.PaginationBasic} />

<p>More options such as <code>first</code>, <code>last</code>, <code>previous</code>, <code>next</code> and <code>ellipsis</code>.</p>
<ReactPlayground codeText={Samples.PaginationAdvanced} />

mtscout6 · 2015-06-08T16:23:15Z

LGTM

Need one more approval from one of the @react-bootstrap/collaborators

AlexKVal · 2015-06-08T17:52:13Z

Address this:

$ npm run lint
src/Pagination.js
  37:28  warning  hasHiddenPagesBefore is defined but never used  no-unused-vars

And this:
className={classNames(this.props.className, this.getBsClassSet())}> here

And we commit 👍

AlexKVal · 2015-06-08T18:07:38Z

@roadmanfong nevemind 😃 , I'll make them myself.

You've done a great job 👍
Thank you 🍒

[added] Pagination component

mtscout6 · 2015-06-08T18:08:33Z

@roadmanfong I agree, huge thanks for putting the effort into this!

Using the `SafeAnchor` for better accessibility. Per #786 commitcomment-11527424 Resolves #788

roadmanfong · 2015-06-09T17:01:04Z

@AlexKVal @mtscout6 Thank you guys. It's a great experience to join this project.

AlexKVal · 2015-06-09T18:34:15Z

@roadmanfong additional PRs are welcome 😉 🌈

mtscout6 · 2015-06-09T23:29:07Z

Agreed, we are always on the lookout for collaborators. Anything on the 1.0.0 roadmap that you'd like to tackle is fair game.

ilyapalkin · 2015-08-04T17:33:37Z

src/Pagination.js

+        key='next'
+        eventKey={this.props.activePage + 1}
+        disabled={this.props.activePage === this.props.items}
+        onSelect={this.props.onSelect}>


Unfortunately, when button is disabled it is still possible to click on it and OnSelect is handled
#1102

Bumps [tar](https://github.com/npm/node-tar) from 6.1.0 to 6.1.5. - [Release notes](https://github.com/npm/node-tar/releases) - [Changelog](https://github.com/npm/node-tar/blob/main/CHANGELOG.md) - [Commits](isaacs/node-tar@v6.1.0...v6.1.5) --- updated-dependencies: - dependency-name: tar dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

roadmanfong mentioned this pull request Jun 4, 2015

Implement Pagination #22

Closed

mtscout6 added this to the 1.0.0 Release milestone Jun 4, 2015

mtscout6 added the enhancement label Jun 4, 2015

stevoland mentioned this pull request Jun 4, 2015

Consider implementing simple components #56

Closed

12 tasks

mtscout6 reviewed Jun 4, 2015
View reviewed changes

mtscout6 mentioned this pull request Jun 4, 2015

[added] createSelectedEvent for consistent onSelect handling #787

Merged

mtscout6 reviewed Jun 4, 2015
View reviewed changes

mtscout6 mentioned this pull request Jun 4, 2015

Replace anchor introduced in Pagination with SafeAnchor #788

Closed

mtscout6 reviewed Jun 4, 2015
View reviewed changes

AlexKVal mentioned this pull request Jun 4, 2015

Use React.findDOMNode() #789

Merged

roadmanfong force-pushed the master branch from e7afd44 to f4bf7a7 Compare June 5, 2015 19:26

roadmanfong force-pushed the master branch from f4bf7a7 to ff68da5 Compare June 5, 2015 19:42

mtscout6 reviewed Jun 5, 2015
View reviewed changes

AlexKVal reviewed Jun 6, 2015
View reviewed changes

roadmanfong force-pushed the master branch from d849aed to ff45f51 Compare June 6, 2015 14:03

roadmanfong force-pushed the master branch from ff45f51 to 4c007a0 Compare June 7, 2015 03:23

[added] Pagination component

5734ec3

roadmanfong force-pushed the master branch from 4c007a0 to 5734ec3 Compare June 7, 2015 19:24

AlexKVal mentioned this pull request Jun 8, 2015

TravisCI should help to discover eslint warnings. #800

Merged

AlexKVal added a commit that referenced this pull request Jun 8, 2015

Merge pull request #786 from roadmanfong/master

1fce364

[added] Pagination component

AlexKVal merged commit 1fce364 into react-bootstrap:master Jun 8, 2015

AlexKVal mentioned this pull request Jun 8, 2015

Address eslint warning. #801

Merged

mtscout6 added a commit that referenced this pull request Jun 9, 2015

[changed] PaginationButton to use SafeAnchor

beaa1fa

Using the `SafeAnchor` for better accessibility. Per #786 commitcomment-11527424 Resolves #788

ilyapalkin reviewed Aug 4, 2015
View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[added] Pagination component #786

[added] Pagination component #786

roadmanfong commented Jun 4, 2015

mtscout6 commented Jun 4, 2015

mtscout6 Jun 4, 2015

mtscout6 Jun 4, 2015

mtscout6 Jun 4, 2015

mtscout6 commented Jun 4, 2015

mtscout6 commented Jun 4, 2015

mtscout6 Jun 4, 2015

mtscout6 Jun 4, 2015

AlexKVal commented Jun 5, 2015

roadmanfong commented Jun 5, 2015

AlexKVal commented Jun 5, 2015

AlexKVal commented Jun 5, 2015

roadmanfong commented Jun 5, 2015

AlexKVal commented Jun 5, 2015

mtscout6 Jun 5, 2015

roadmanfong commented Jun 6, 2015

AlexKVal Jun 6, 2015

mtscout6 Jun 6, 2015

AlexKVal commented Jun 6, 2015

mtscout6 commented Jun 6, 2015

roadmanfong commented Jun 7, 2015

roadmanfong commented Jun 8, 2015

mtscout6 commented Jun 8, 2015

AlexKVal commented Jun 8, 2015

AlexKVal commented Jun 8, 2015

mtscout6 commented Jun 8, 2015

roadmanfong commented Jun 9, 2015

AlexKVal commented Jun 9, 2015

mtscout6 commented Jun 9, 2015

ilyapalkin Aug 4, 2015

[added] Pagination component #786

[added] Pagination component #786

Conversation

roadmanfong commented Jun 4, 2015

mtscout6 commented Jun 4, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtscout6 commented Jun 4, 2015

mtscout6 commented Jun 4, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexKVal commented Jun 5, 2015

roadmanfong commented Jun 5, 2015

AlexKVal commented Jun 5, 2015

AlexKVal commented Jun 5, 2015

roadmanfong commented Jun 5, 2015

AlexKVal commented Jun 5, 2015

Choose a reason for hiding this comment

roadmanfong commented Jun 6, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexKVal commented Jun 6, 2015

mtscout6 commented Jun 6, 2015

roadmanfong commented Jun 7, 2015

roadmanfong commented Jun 8, 2015

mtscout6 commented Jun 8, 2015

AlexKVal commented Jun 8, 2015

AlexKVal commented Jun 8, 2015

mtscout6 commented Jun 8, 2015

roadmanfong commented Jun 9, 2015

AlexKVal commented Jun 9, 2015

mtscout6 commented Jun 9, 2015

Choose a reason for hiding this comment