Fixes #123 - add PaginationRow component #162

tstrachota · 2018-01-11T13:59:57Z

Adds PaginationRow component that implements Patternfly pagination:
https://rawgit.com/patternfly/patternfly/master-dist/dist/tests/pagination.html

Usage:

<PaginationRow
    totalCount={75}
    perPage={10}
    currentPage={2}
    perPageOptions={[5, 10, 15]}
    onPageSet={action('page set')}
    onPerPageSet={action('per page value set')}
/>

priley86 · 2018-01-11T14:13:23Z

@tstrachota thanks for this - I am happy to compare/contrast with what I've started in #143. I'll try to accommodate anything I am missing here. Also - feel free to chime in with any issues...

priley86 · 2018-01-11T14:57:18Z

src/components/PaginationRow/PaginationRow.js

+
+import { MenuItem, DropdownButton } from '../../index';
+
+const defaultMessages = {


👍 . I like this addition of making the message text a parameter with default prop values. Will also add this in #143

this is a must for i18n... so thanks again for this. I'll continue to be on the lookout for these kind of things.

ohadlevy · 2018-01-11T15:02:02Z

src/components/PaginationRow/PaginationRow.js

+
+  handleFormSubmit(e) {
+    this.setPage(this.state.pageChangeValue);
+    e.preventDefault();


dont think it matters much (or at all), but I would make sure the browser doesn't submit first :)

also, shouldn't we allow to execute a custom action? e.g. actually go fetch new data or something?

ya i don't think we need to handle form submit... just handle the callback when the values change...and have the server go fetch it...and the send the props back down.

It actually prevents the form from being submitted. I didn't find any way how to use onChange only, because it's triggered on every key-up -> you can't enter more than one-digit numbers.

this.setPage executes onPageSet callback that can be injected via props and used to trigger new data fetch.

tstrachota · 2018-01-12T11:24:30Z

Updated. handlePageChange was raising exception for undefined this.

jeff-phillips-18 · 2018-01-12T12:28:17Z

src/components/PaginationRow/PaginationRow.js

+    return (
+      <div>
+        <form
+          className="content-view-pf-pagination clearfix"


To make this usable with a table view, we need a mechanism to add .table-view-pf-pagination here.

I added a className prop to PaginationRow

jeff-phillips-18 · 2018-01-12T12:33:03Z

src/components/PaginationRow/PaginationRow.js

+                  title={this.msg('firstPage')}
+                  onClick={() => this.setPage(1)}
+                >
+                  <span className="i fa fa-angle-double-left" />


This and others following can use the <Icon> component.

jeff-phillips-18 · 2018-01-12T12:34:35Z

src/components/PaginationRow/PaginationRow.js

+  currentPage: PropTypes.number.isRequired,
+  onPerPageSet: PropTypes.func,
+  onPageSet: PropTypes.func,
+  messages: PropTypes.object


Can we add comments to show the list messages in storybook?

tstrachota · 2018-01-15T13:53:34Z

Updated. I also added a storybook example with messages.

waldenraines · 2018-01-17T15:52:43Z

Updated. I also added a storybook example with messages.

Can you provide a link to the storybook as detailed here: https://github.com/patternfly/patternfly-react/#storybook-ui-development

sharvit

Thanks @tstrachota, looks pretty good!
Can you deploy the storybook please?

sharvit · 2018-01-18T09:17:36Z

src/components/PaginationRow/PaginationRow.js

+
+const ArrowIcon = props => {
+  const name = `angle-${props.name}`;
+  return <Icon type="fa" name={name} className="i" />;


Can you move the ArrowIcon into src/components/PaginationRow/InnerComponents/ArrowIcon.js as described here:
https://github.com/patternfly/patternfly-react/blob/master/CONTRIBUTING.md#code-consistency

i think we can expose it ;)

sharvit · 2018-01-18T09:31:14Z

src/components/PaginationRow/PaginationRow.js

+  onPageSet: p => {},
+  onPerPageSet: pp => {},
+  messages: {},
+  className: 'content-view-pf-pagination'


Passing a className will override the content-view-pf-pagination, shouldn't it always be there?
Checkout this pr #163

You can add another prop called baseClassName that can have content-view-pf-pagination as default value.
So consumers can add more classes when using className and override the base class when passing baseClassName.

sharvit · 2018-01-18T09:32:33Z

src/components/PaginationRow/PaginationRow.js

+  /** A callback triggered when a page is switched */
+  onPageSet: PropTypes.func,
+  /** Strings in the component, see PaginationRow.defaultMessages for details */
+  messages: PropTypes.object,


Can you use PropTypes.shape here?

sharvit · 2018-01-18T09:39:27Z

src/components/PaginationRow/PaginationRow.stories.js

+
+stories.addWithInfo('With translations', '', () => {
+  var messages = {};
+  for (var key in PaginationRow.defaultMessages) {


In today javascript, we should never use for...in syntax again, it is actually danger.
Please use for...of which is an es2015 syntax.
https://www.eventbrite.com/engineering/learning-es6-for-of-loop/

jeff-phillips-18 · 2018-01-18T14:43:41Z

@tstrachota what are your thoughts on the pagination row being done as part of #75? Does that remove the need for this PR? Does that contain everything you wanted from this PR?

jgiardino · 2018-01-18T14:55:00Z

@jeff-phillips-18 Did you mean #143?
Is the pagination in that example specific to tables @priley86, or could it be reused for other views, like the list view?

jeff-phillips-18 · 2018-01-18T15:00:58Z

Yes, sorry #143 (we should probably close #75)

tstrachota · 2018-01-23T12:47:19Z

@jeff-phillips-18 I'd like to send as PR into @priley86's branch that adds my functionality on the top of his components.

jeff-phillips-18 · 2018-01-23T15:09:25Z

Ok @tstrachota sounds good. Once that is done, can you close this PR?

tstrachota · 2018-01-24T13:11:10Z

OK, @priley86 copied my code to his branch over my PTO. There's no sense in doing the PR now. I give up. Closing this PR. Will at least try to review the other PR.

priley86 reviewed Jan 11, 2018

View reviewed changes

ohadlevy reviewed Jan 11, 2018

View reviewed changes

priley86 mentioned this pull request Jan 12, 2018

feat(tables): adds reactabular and patternfly tables examples #143

Merged

jeff-phillips-18 reviewed Jan 12, 2018

View reviewed changes

Fixes #123 - add PaginationRow component

008ac7f

sharvit suggested changes Jan 18, 2018

View reviewed changes

tstrachota closed this Jan 24, 2018

AllenBW mentioned this pull request Feb 20, 2018

Enhancement - Pagination previous/next paging #217

Closed


		import { MenuItem, DropdownButton } from '../../index';

		const defaultMessages = {

Fixes #123 - add PaginationRow component #162

Fixes #123 - add PaginationRow component #162

Uh oh!

Conversation

tstrachota commented Jan 11, 2018

Uh oh!

priley86 commented Jan 11, 2018

Uh oh!

priley86 Jan 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tstrachota commented Jan 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tstrachota commented Jan 15, 2018

Uh oh!

waldenraines commented Jan 17, 2018

Uh oh!

sharvit left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeff-phillips-18 commented Jan 18, 2018

Uh oh!

jgiardino commented Jan 18, 2018

Uh oh!

jeff-phillips-18 commented Jan 18, 2018

Uh oh!

tstrachota commented Jan 23, 2018

Uh oh!

jeff-phillips-18 commented Jan 23, 2018

Uh oh!

tstrachota commented Jan 24, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

priley86 Jan 11, 2018 •

edited

Loading