[Pagination] Add TypeScript types #19933
Conversation
oliviertassinari
added
the
component: pagination
Details of bundle changes.Comparing: 91fa13b...4ef9bea Details of page changes
|
|
|
|
@pvdstel You can click on the "Details" link in the checks tab to find out the issue. |
|
Managed to get this working. I had to actually 'use' |
oliviertassinari
changed the title
|
Not sure where this is coming from: |
|
|
|
Ah, thanks! (As you'll have probably gathered from the absence of type definitions for this component, TypeScript is not my thing, although I should probably do something about that sooner or later. Not a first – I ignored CSS for a long time after it was first added to browsers! 😆) Odd that you had to 'use' @eps1lon Thoughts? |
|
6f4b0c9 (most recent commit) should remove the @mbrookes generateProptypes.ts has a hardcoded check for |
| * | ||
| * For localization purposes, you can use the provided [translations](/guides/localization/). | ||
| * | ||
| * @param {string} [type = page] The link or button type to format ('page' | 'first' | 'last' | 'next' | 'previous'). |
There was a problem hiding this comment.
Could you remove the types in the jsdoc? They duplicate the TS types and conflict in this case. It's also not clear when type can be undefined while also having a default value.
There was a problem hiding this comment.
Ah, I thought that syntax somehow implied an optional argument. Will fix it in a sec.
There was a problem hiding this comment.
I can't remove the types from the jsdoc here. These docs are copied to the JS component by yarn proptypes --disable-cache, and then the yarn docs:api command will fail because there is no type annotation.
There was a problem hiding this comment.
I looked into adding support for this upstream, the parsing of basic types is complete but I stopped after a limited search for a JSDoc -> AST -> JSDoc lib failed.
oliviertassinari
added
the
PR: needs revision
It is removed?
Yeah, but oddly only in CI... it doesn't create any changes when running it locally. 🤷♂
Yep, I missed a merge conflict when merging master. Already fixed. |
@eps1lon @pvdstel The reason it does static analysis of the implementation is explained here #16521 (comment) |
The
I do get the same behavior, but the CI runs it as |
Pagination now extends UsePagination; inherited props have been removed.
docs/pages/api-docs/pagination.md
Outdated
| | <span class="prop-name">hideNextButton</span> | <span class="prop-type">bool</span> | <span class="prop-default">false</span> | If `true`, hide the next-page button. | | ||
| | <span class="prop-name">hidePrevButton</span> | <span class="prop-type">bool</span> | <span class="prop-default">false</span> | If `true`, hide the previous-page button. | | ||
| | <span class="prop-name">onChange</span> | <span class="prop-type">func</span> | | Callback fired when the page is changed.<br><br>**Signature:**<br>`function(event: object, page: number) => void`<br>*event:* The event source of the callback.<br>*page:* The page selected. | | ||
| | <span class="prop-name">page</span> | <span class="prop-type">number</span> | | The current page. | | ||
| | <span class="prop-name">renderItem</span> | <span class="prop-type">func</span> | <span class="prop-default">item => <PaginationItem {...item} /></span> | Render the item.<br><br>**Signature:**<br>`function(params: object) => ReactNode`<br>*params:* The props to spread on a PaginationItem. | | ||
| | <span class="prop-name">renderItem</span> | <span class="prop-type">func</span> | <span class="prop-default">item => <PaginationItem {...item} /></span> | The pagination item component to render.<br><br>**Signature:**<br>`function(params: object) => ReactNode`<br>*params:* The props to spread on the component. | |
There was a problem hiding this comment.
It's not a component e.g. you couldn't use hooks inside it. Please make sure the documentation does not change in this PR (unless it's just typos).
Sorry, you're right. ("Too many cooks..." Edit: Specifically referring to myself here!) |
|
Hi, any updates here? Should anything be done before it can be merged? |
|
@pvdstel Much appreciated, thanks. Fell under the radar since we're a bit stretched at the moment. |
We're currently experimenting with the Pagination component, and we think it's a really nice component. Since we use TypeScript mainly, we would really like this component to have a TypeScript interface as well.
This pull request would add:
Paginationcomponent;PaginationItemcomponent;usePaginationfunction.I have attempted to create these type definitions such that they closely resemble those of existing components. For Pagination and PaginationItem, the prop types are based on the propTypes defined below the components. For usePagination, I have assumed these types based on the Pagination and PaginationItem props.