Skip to content

Conversation

@ivanjeremic
Copy link

no need to call fallback()

no need to call fallback()
@seppwc
Copy link
Owner

seppwc commented Jun 11, 2022

Hi thanks for the PR.

Ideally things which are api changes I would prefer to have an issue made first. but thats my fault for not have a contributors guide.

Im happy to go this route though, as after testing this pattern on a prototype doesn't seem to have any major problems with this implementation, and i guess is more consistent with how other libraries deal with passing components as props.

the main problem setting this back is that the tests, types (fallback prop) and storybook are all broken because of this change. These will need to be updated/fixed before this gets merged

Show › renders fallback if "when" is falsy

    expect(received).toBeTruthy()

    Received: null

      37 |     setup({when: false, fallback: FallBack});
      38 |     expect(screen.queryByText(Text.default)).toBeNull()
    > 39 |     expect(screen.queryByText(Text.fallback)).toBeTruthy()
         |                                               ^
      40 |   });
      41 |
      42 |   it('renders children if "when" is true, with fallback', () => {

      at Object.<anonymous> (src/components/Show/Show.test.tsx:39:47)

@seppwc seppwc mentioned this pull request Jun 11, 2022
@seppwc seppwc added the enhancement New feature or request label Jun 11, 2022
@seppwc
Copy link
Owner

seppwc commented Jun 23, 2022

@ivanjeremic

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants