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

Shared type PropsBasedOnComponent #154

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

lucasmaffazioli
Copy link
Contributor

@lucasmaffazioli lucasmaffazioli commented Mar 7, 2023

Pull Request

Description

While developing Pagination, it was created a type for getting props based on a passed component props.

In the below example, component receives Link, which has the link prop, which then is available on PaginationItem, with this, we have PropsBasedOnComponent.

image

This is also called Polymorphic component.

This change is only to include this generic type for Grid.Col as well.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Documentation change

How do I test this

  • Since the change is only for sharing the same type, the only test needed is to see if SB is ok and tests are passing.

Checklist

Did you remember to take care of the following?

  • npm i – for new NPM dependencies.
  • npm run lint - to check for linting issues
  • npm run test - to run unit tests
  • npm run test:sh:docker - to run screenshot tests, detail instruction

New Feature / Bug Fix

  • Run unit tests to ensure all existing tests are still passing.
  • Add new passing unit tests to cover the code introduced by your pr.

Thanks for contributing!

@lucasmaffazioli lucasmaffazioli added the work in progress This is a work in progress label Mar 7, 2023
@lucasmaffazioli lucasmaffazioli self-assigned this Mar 7, 2023
@github-actions
Copy link

github-actions bot commented Mar 7, 2023

Compiled a new version demo.

@github-actions
Copy link

github-actions bot commented Mar 7, 2023

Compiled a new version demo.

@github-actions
Copy link

github-actions bot commented Mar 7, 2023

Compiled a new version demo.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 4355349209

  • 2 of 2 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.003%) to 95.471%

Totals Coverage Status
Change from base Build 4346789629: 0.003%
Covered Lines: 1083
Relevant Lines: 1102

💛 - Coveralls

@github-actions
Copy link

github-actions bot commented Mar 7, 2023

Compiled a new version demo.

@github-actions
Copy link

github-actions bot commented Mar 7, 2023

Coverage report

St.
Category Percentage Covered / Total
🟢 Statements 97.7% 1190/1218
🟢 Branches 85.53% 266/311
🟢 Functions 94.79% 91/96
🟢 Lines 98.28% 1083/1102

Test suite run success

273 tests passing in 41 suites.

Report generated by 🧪jest coverage report action from 4654fa5

@lucasmaffazioli lucasmaffazioli marked this pull request as ready for review March 7, 2023 15:15
@auto-assign auto-assign bot requested review from esraltintas and nathanjamal March 7, 2023 15:15
@lucasmaffazioli lucasmaffazioli requested review from frankie303 and removed request for nathanjamal and esraltintas March 7, 2023 15:15
@lucasmaffazioli lucasmaffazioli removed the work in progress This is a work in progress label Mar 7, 2023
Copy link
Contributor

@frankie303 frankie303 left a comment

Choose a reason for hiding this comment

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

great PR! here is my review:

Comment on lines +12 to +13
// export Col is necessary with PropsBasedOnComponent due to a Storybook bug
export const Col: PropsBasedOnComponent<ColProps, 'div'> = ({
Copy link
Contributor

Choose a reason for hiding this comment

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

should we keep the default export in this case (IMO we shouldn't) and what is the Storybook bug?

type ColBaseProps<E extends React.ElementType> = {
export interface ColProps {
Copy link
Contributor

Choose a reason for hiding this comment

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

can you also update Row and Container as well?

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