Introduce react pager component #11802

stacey-gammon · 2017-05-15T19:21:25Z

weltenwort

Feels good to read such a nice and clean set of React components! 😄 I left a few small questions below.

weltenwort · 2017-05-16T12:22:49Z

ui_framework/components/pager/pager.js

+}
+
+KuiPager.propTypes = {
+  startNumber: React.PropTypes.number,


Should those numbers be required as well?

weltenwort · 2017-05-16T12:27:58Z

ui_framework/components/pager/pager.js

+    endNumber,
+    totalItems,
+    hasPreviousPage,
+    hasNextPage,


Is there a use case in which hasPreviousPage and hasNextPage should not be derived from startNumber, endNumber and totalItems? If not, they could be calculated in here as startNumber > 1 and endNumber < totalItems respectively.

That might not actually always be true. For instance, the add panel pager:

endNumber < totalItems but hasNext is false. likewise I could potentially see someone wanting to loop back around to the end if they kept hitting previous.

weltenwort · 2017-05-16T12:33:11Z

ui_framework/components/pager/pager_buttons.js

+        type="basic"
+        onClick={onPrevious}
+        disabled={!hasPrevious}
+        icon={<KuiButtonIcon type="previous" />}


Do we need to specify an aria-label for buttons that only have an icon (which is aria-hidden)?

I was thinking the same thing. This button should have aria-label="Show previous page" and the other button should have aria-label-"Show next page".

weltenwort · 2017-05-16T12:43:08Z

ui_framework/doc_site/src/services/routes/routes.js

@@ -160,6 +163,9 @@ const components = [{
  component: ModalExample,
  hasReact: true,
 }, {
+  name: 'Pager',
+  component: PagerExample,


hasReact: true, would be justified here

@stacey-gammon I just added this feature; with hasReact: true, a little React logo will show up next to the navigation item for this example.

cjcenizal

This is FANTASTIC! I love how our React components keep growing. I just had a couple small suggestions.

cjcenizal · 2017-05-16T17:26:48Z

ui_framework/components/index.js

+
+export {
+  KuiPager,
+  KuiPagerButtons


Can we rename this KuiPagerButtonGroup, to create an analogy with the ButtonGroup component?

cjcenizal · 2017-05-16T17:31:35Z

ui_framework/components/pager/pager.js

+    hasNextPage,
+    onNextPage,
+    onPreviousPage,
+    ...rest }) {


I hate to nitpick syntax, but can we indent the closing syntax onto the next line, and add a dangling comma here?

...rest, }) {

It makes the parameter list feel more enclosed to me, so I can identify it and differentiate it from the body of the function definition more easily. The dangling comma also adheres to AirBnB's style.

cjcenizal · 2017-05-16T17:32:57Z

ui_framework/components/pager/pager.js

+    <div className={classes} { ...rest }>
+      <div className="kuiPagerText">{startNumber}&ndash;{endNumber} of {totalItems}</div>
+      {
+        startNumber === 1 && endNumber === totalItems


For anyone not quite sure about the order of operations when it comes to evaluating ternaries, how about wrapping this condition in parens?

(startNumber === 1 && endNumber === totalItems)

cjcenizal · 2017-05-16T17:37:44Z

ui_framework/components/pager/pager_buttons.js

+        type="basic"
+        onClick={onPrevious}
+        disabled={!hasPrevious}
+        icon={<KuiButtonIcon type="previous" />}


I was thinking the same thing. This button should have aria-label="Show previous page" and the other button should have aria-label-"Show next page".

cjcenizal · 2017-05-16T17:39:21Z

ui_framework/doc_site/src/services/routes/routes.js

@@ -160,6 +163,9 @@ const components = [{
  component: ModalExample,
  hasReact: true,
 }, {
+  name: 'Pager',
+  component: PagerExample,


@stacey-gammon I just added this feature; with hasReact: true, a little React logo will show up next to the navigation item for this example.

cjcenizal · 2017-05-16T17:39:55Z

ui_framework/doc_site/src/views/pager/pager_example.js

+      }]}
+    >
+      <GuideText>
+        Use the Pager in a tool bar


Can we add a period at the end of this sentence?

cjcenizal · 2017-05-16T17:40:01Z

ui_framework/doc_site/src/views/pager/pager_example.js

+      }]}
+    >
+      <GuideText>
+        Use the Pager Buttons to navigate through a set of items


A period here, too?

stacey-gammon · 2017-05-22T15:25:59Z

@cjcenizal, @weltenwort all changes and questions should be addressed now. Thanks!

cjcenizal

One small thing I missed originally, then LGTM! 🎺

cjcenizal · 2017-05-22T17:29:17Z

ui_framework/components/pager/pager_button_group.js

+        type="basic"
+        onClick={onPrevious}
+        disabled={!hasPrevious}
+        icon={<KuiButtonIcon type="previous" />}


Can we add aria-hidden="true" here? This will prevent screen readers from trying to read the Font Awesome icon, since it's technically text, just using an icon font. If/when we switch to SVG icons this won't matter.

Actually never mind, the React component does this automatically. Sweeeet.

cjcenizal · 2017-05-22T17:29:36Z

ui_framework/components/pager/pager_button_group.js

+        type="basic"
+        onClick={onNext}
+        disabled={!hasNext}
+        icon={<KuiButtonIcon type="next" />}


aria-hidden-"true" here too

Ignore this too

stacey-gammon · 2017-05-26T13:29:59Z

@weltenwort any more feedback or is this good to go? Thanks!

* Introduce react pager component * Rename KuiPagerButtons to KuiPagerButtonGroup * Address code review comments * update snapshots with new aria labels. * fix merge issue

tsullivan · 2017-06-14T19:45:52Z

I just found this and it's awesome! I will definitely use it in #12150

stacey-gammon · 2017-06-14T19:49:13Z

omg @tsullivan I think I just stepped on your toes -- #12349 -- I didn't realize you were reactifying stuff too.

tsullivan · 2017-06-14T20:53:04Z

@stacey-gammon no worries! "Reactifying stuff" is an overstatement for what I've been doing. I think we should sync up though and share some ideas.

stacey-gammon added the Team:Platform-Design Team Label for Kibana Design Team. Support the Analyze group of plugins. label May 15, 2017

stacey-gammon requested review from cjcenizal and weltenwort May 15, 2017 20:18

stacey-gammon added review v5.5.0 labels May 15, 2017

weltenwort reviewed May 16, 2017

View reviewed changes

cjcenizal suggested changes May 16, 2017

View reviewed changes

stacey-gammon added 3 commits May 19, 2017 09:56

Introduce react pager component

276784e

Rename KuiPagerButtons to KuiPagerButtonGroup

9d36460

Address code review comments

344a3bb

stacey-gammon force-pushed the pager-in-react branch from f2c9709 to 344a3bb Compare May 19, 2017 13:56

stacey-gammon added 2 commits May 19, 2017 11:04

update snapshots with new aria labels.

384c4fe

fix merge issue

fc59844

stacey-gammon added the :Sharing label May 22, 2017

cjcenizal approved these changes May 22, 2017

View reviewed changes

stacey-gammon merged commit 7b1ad58 into elastic:master May 30, 2017

This was referenced May 30, 2017

[5.X] Introduce react pager component (#11802) #12070

Merged

Use new buttonType instead of older type attribute #12071

Merged

stacey-gammon deleted the pager-in-react branch October 24, 2017 13:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce react pager component #11802

Introduce react pager component #11802

stacey-gammon commented May 15, 2017

weltenwort left a comment •

edited

Loading

weltenwort May 16, 2017

weltenwort May 16, 2017

stacey-gammon May 16, 2017

weltenwort May 16, 2017

cjcenizal May 16, 2017

weltenwort May 16, 2017

cjcenizal May 16, 2017

cjcenizal left a comment

cjcenizal May 16, 2017

cjcenizal May 16, 2017

cjcenizal May 16, 2017

cjcenizal May 16, 2017

cjcenizal May 16, 2017

cjcenizal May 16, 2017

cjcenizal May 16, 2017

stacey-gammon commented May 22, 2017

cjcenizal left a comment

cjcenizal May 22, 2017

cjcenizal May 22, 2017

cjcenizal May 22, 2017

cjcenizal May 22, 2017

stacey-gammon commented May 26, 2017

tsullivan commented Jun 14, 2017

stacey-gammon commented Jun 14, 2017

tsullivan commented Jun 14, 2017

Introduce react pager component #11802

Introduce react pager component #11802

Conversation

stacey-gammon commented May 15, 2017

weltenwort left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjcenizal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stacey-gammon commented May 22, 2017

cjcenizal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stacey-gammon commented May 26, 2017

tsullivan commented Jun 14, 2017

stacey-gammon commented Jun 14, 2017

tsullivan commented Jun 14, 2017

weltenwort left a comment •

edited

Loading