Skip to content
This repository has been archived by the owner on Aug 18, 2020. It is now read-only.

feat(controls): add pagination controls #12

Merged
merged 6 commits into from
Feb 22, 2017
Merged

feat(controls): add pagination controls #12

merged 6 commits into from
Feb 22, 2017

Conversation

eddier
Copy link
Contributor

@eddier eddier commented Feb 18, 2017

feature description

Adding pagination controls to the the set of available components

content

Includes the PaginationControl element as well as the necessary CSS. Also includes testable storybook component. No new dependencies.

usage

Handle the navigateToPage call:

navigateToPage(page) { this.setState({ currentNotes: this.getNotes(page) }); }

In this example you can fetch the appropriate notes from the API or if they are all loaded in a parent array just set them...

getNotes(pageNum) { return this.state.allNotes.slice( pageNum * this.state.perPage - this.state.perPage, pageNum * this.state.perPage); }

@cdaringe
Copy link
Contributor

Storybook Hub is building storybooks for every commit in this PR.

Latest Storybook: https://38dfe732-fe93-4b01-8e82-343b109a5f83.sbook.io/
( Visit here for more info )

This comment is automatically added by Storybook Hub

super(props)
this.state = {
isPrevPageValid: false,
isNextPageValid: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

i think this is fine, just brainstorming. both of these ^^ are derived state. hence, something akin to get isPrevPageValid () { this.state.currentPage > 1 } and the like could feasibly 🔥 down some code! no action requested. just talkin :)

</div>
);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be removed, i think. we already have a Welcome component

@cdaringe
Copy link
Contributor

hey @eddier, looks good.

  • i had a few remarks. will you 👀 at those?
  • can you see the build error messages? will want to 💚 that little guy pre merge!
  • on the Pagination component,
    • i was clicking the page buttons and the text kept selecting/highlighting (because of my aggressivf clicking!). maybe we can drop these on?? what do you think?
    • were things like "jump to end/start" or "select items per page" requirements? just askin!

thanks! this is cool stuff! @antoinejuhel, 👀 storybook

@cdaringe
Copy link
Contributor

oh, and, https://github.com/semantic-release/semantic-release#default-commit-message-format . dont worry about rebasing. were still learning it too!

@eddier
Copy link
Contributor Author

eddier commented Feb 21, 2017

Can you give me account on your Codeship instance so I can see the build issues?

I'll check out the triple click issue that is causing the highlighting, good catch. The "jump to end" state was not part of the initial scope but it makes sense to include those as optional controls. I'll bring it up to the design team. I assume you guys will handle the required API support necessary for those components?

This resolves the text selecting from rapidly triple clicking
@cdaringe
Copy link
Contributor

cdaringe commented Feb 21, 2017

cool. hmm. the project should be open. i just double checked. do you have an acct? looks like the linter isnt happy atm

@cdaringe
Copy link
Contributor

& yep, we'll wire in the API calls. thx!

@eddier
Copy link
Contributor Author

eddier commented Feb 21, 2017

eraffaele at g mail - it currently throws an error about access being denied to CS.

@jhegg
Copy link
Contributor

jhegg commented Feb 22, 2017

@eddier yarn run lint should show the errors, until you get access to CS. I'm able to repro locally.

$ yarn run lint
yarn run v0.20.3
$ standard 'src/**/*.js' '.storybook/**/*.js' 'scripts/**/*.js' 'test/**/*.js' 'stories/**/*.js' 
standard: Use JavaScript Standard Style (http://standardjs.com)
standard: Run `standard --fix` to automatically fix some problems.
  /data/src/octagon/stories/controls.stories.js:36:30: Trailing spaces not allowed.
  /data/src/octagon/stories/controls.stories.js:39:18: Missing space before function parentheses.
  /data/src/octagon/stories/controls.stories.js:39:20: Missing space before opening brace.
  /data/src/octagon/stories/controls.stories.js:40:59: Extra semicolon.
  /data/src/octagon/stories/controls.stories.js:45:17: Expected consistent spacing
  /data/src/octagon/stories/controls.stories.js:144:25: Trailing spaces not allowed.
  /data/src/octagon/stories/controls.stories.js:145:49: Trailing spaces not allowed.
  /data/src/octagon/stories/controls.stories.js:146:20: Trailing spaces not allowed.
  /data/src/octagon/stories/controls.stories.js:147:34: Trailing spaces not allowed.
  /data/src/octagon/stories/controls.stories.js:148:33: Trailing spaces not allowed.
  /data/src/octagon/stories/controls.stories.js:153:1: Trailing spaces not allowed.
error Command failed with exit code 1.

This resolves the linting issues which may be causing CI to fail
@eddier
Copy link
Contributor Author

eddier commented Feb 22, 2017

Thanks Josh! Didn't realize the .stories.js files were in scope of the linter for your ci.

@cdaringe
Copy link
Contributor

cdaringe commented Feb 22, 2017 via email

@jhegg
Copy link
Contributor

jhegg commented Feb 22, 2017

I noticed that the "5 of 15" page number text has the text selection cursor when mousing over; should that just have the normal pointing mouse cursor instead? (I'm really not sure)

keep the cursor as default when hovering over the page count
@eddier
Copy link
Contributor Author

eddier commented Feb 22, 2017

That makes sense to me, updated to reflect that change.

@cdaringe cdaringe merged commit 4caa0a3 into master Feb 22, 2017
@cdaringe cdaringe deleted the pagination branch February 22, 2017 18:01
@antoinejuhel
Copy link

Because of the sorting functions we have for consoles, I don't see a need for jump to end/beginning in this use case. That said if these are the pagination controls we want to use throughout our products, then yes, a jump to end/beginning would be required (a good example of this would be for reporting).

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

Successfully merging this pull request may close these issues.

4 participants