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

Adds a Paginator (Pagination Component) #101

Merged
merged 64 commits into from
Jan 15, 2020
Merged

Adds a Paginator (Pagination Component) #101

merged 64 commits into from
Jan 15, 2020

Conversation

peterMuriuki
Copy link
Collaborator

@peterMuriuki peterMuriuki commented Sep 12, 2019

Adds a Headless Pagination Component

paginator

@peterMuriuki
Copy link
Collaborator Author

Blocked:

me@thinkpad-t440s ~/D/o/js-tools (pagination)> 
yarn babel src -d dist --root-mode upward --extensions '.ts,.tsx'  --ignore '**/*.test.ts,**/*.test.tsx,**/tests,**/__tests__'
yarn run v1.12.3
$ /home/peter/Desktop/onaio/js-tools/node_modules/.bin/babel src -d dist --root-mode upward --extensions .ts,.tsx --ignore '**/*.test.ts,**/*.test.tsx,**/tests,**/__tests__'
babel:
  src does not exist
error Command failed with exit code 2.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

Package.json script section:

"scripts": {
    "test": "jest",
    "lint-js": "eslint '{packages,stories}/**/*.{js,jsx}'",
    "lint-ts": "tslint -c tslint.json '{packages/**/src,stories}/**/*.{ts,tsx}'",
    "storybook": "start-storybook -p 6006",
    "build-storybook": "build-storybook",
    "clean-build": "rimraf packages/**/dist"
  }

me: 😕

@peterMuriuki
Copy link
Collaborator Author

@moshthepitt ☝️

@peterMuriuki
Copy link
Collaborator Author

Also mpower might need this very soon, if not already. Am hoping that we can get this published as soon as possible after the above is fixed and the review.

@moshthepitt
Copy link
Contributor

@p-netm please ping me when you're ready for a review

@moshthepitt
Copy link
Contributor

@p-netm the babel command and the tsc command should be run from within the individual package's directory. Could you please update the README in this PR?

Copy link
Contributor

@moshthepitt moshthepitt left a comment

Choose a reason for hiding this comment

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

Why is there no dist/index.js file - imho we should have this 1) for consistency and 2) it is specified in package.json?

Copy link
Contributor

@moshthepitt moshthepitt left a comment

Choose a reason for hiding this comment

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

Can we remove the lodash dependency? I think we are only using the range function, which we can trivially define - no?

e.g.

const range = (start: number, stop: number, step = 1) =>
    Array(Math.ceil((stop - start) / step)).fill(start).map((x, y) => x + y * step)

Copy link
Contributor

@moshthepitt moshthepitt left a comment

Choose a reason for hiding this comment

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

What is the reason for not making this work via React Router, instead of using click handlers?

packages/Pagination/src/Paginator/index.tsx Outdated Show resolved Hide resolved
@peterMuriuki
Copy link
Collaborator Author

Why is there no dist/index.js file - imho we should have this 1) for consistency and 2) it is specified in package.json?

@moshthepitt is the file dist/index.js auto generated during transpiling?

@moshthepitt
Copy link
Contributor

@p-netm for that to happen you need an index.tsx in the root package directory.

Copy link
Contributor

@moshthepitt moshthepitt left a comment

Choose a reason for hiding this comment

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

@p-netm is there a test for RoutedPaginator that shows rendered Link components?

packages/Pagination/src/Paginator/tests/Paginator.test.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@moshthepitt moshthepitt left a comment

Choose a reason for hiding this comment

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

@p-netm I really like this "headless" approach. Good job.

I think what's left is:

  • Add docstrings to Paginator/index.tsx
  • Add documentation about this approach being headless, what that means, how to work with it, and an example (probably best to go with a bootstrap example)

@peterMuriuki peterMuriuki merged commit 3f53c16 into master Jan 15, 2020
@moshthepitt moshthepitt deleted the pagination branch January 30, 2020 07:51
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.

3 participants