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

Invalid prop label supplied to InputGroup #492

Closed
ivarni opened this issue Nov 7, 2018 · 8 comments
Closed

Invalid prop label supplied to InputGroup #492

ivarni opened this issue Nov 7, 2018 · 8 comments
Assignees

Comments

@ivarni
Copy link
Contributor

ivarni commented Nov 7, 2018

Which package is this issue related to?

ffe-form-react

Describe your issue (screenshots welcome!)

I am getting an "Invalid prop label supplied to InputGroup" message when passing the exported Label component to the label prop of InputGroup

What is the expected behavior?

Since the documentation at https://design.sparebank1.no/styleguidist/index.html#!/InputGroup says I am allowed to pass a Label component I don't expect any console warnings.

What is the actual behavior?

I get console warnings.

Steps to reproduce

<InputGroup label={
    <Label>
        <strong className="ffe-strong-text">
            Label strong. So strong.
        </strong>
    </Label>
}>
    <Input name="foo" />
</InputGroup>

Do note that doing this in the actual designguide does not produce a warning, but doing it in a consuming application does (At least on 4.2.0).

Suggestion: Relax the prop-check to be node/string rather than custom/string which it is today.

@selbekk
Copy link
Contributor

selbekk commented Nov 22, 2018

Here's your code in a Sandbox: https://codesandbox.io/s/x786o09k0q

No warnings there. Not sure how you're getting an error here - are you wrapping the Label component in another component or something?

If we relax the prop types, we run the risk of somebody passing in a non-<label /> based component, which will then receive the htmlFor prop which is "magically" applied. That'll throw a warning about the "htmlFor" property not being supported for the given HTML node.

@ivarni
Copy link
Contributor Author

ivarni commented Nov 22, 2018

I've no idea, haven't had time to look closer at it. If someone passes a non-label it would, as you say throw a warning anyway.

@ivarni
Copy link
Contributor Author

ivarni commented Nov 23, 2018

I'm basically doing that exact same thing as the codesandbox and I'm also getting a warning when doing this with Tooltip (which really should be a Tooltip component and nothing else).

The Label stuff is gone now, but I do still get a warning for the tooltip prop and the debugger seems to agree with me that I've provided a Tooltip

image

So what's going on here then?

The import is

import { InputGroup, Tooltip } from '@sb1/ffe-form-react';

But there seems to be some mischief with my version, output from npm ls is

├─┬ @sb1/ffe-radio-button-react@3.3.2red by node-pre-gyp@0.10.0
│ ├── UNMET PEER DEPENDENCY @sb1/ffe-core@^14.0.2irp@0.5.1
│ ├── UNMET PEER DEPENDENCY @sb1/ffe-form@^10.1.1nore-walk@3.0.1
│ ├─┬ @sb1/ffe-form-react@4.2.2on@1.1.11, required by minimatch@3.0.4

and

├─┬ @sb1/ffe-form-react@4.2.0
│ ├── classnames@2.2.6 deduped
│ ├── react-css-collapse@3.6.0
│ └── uuid@3.3.2 deduped

So I seem to be running two different versions here. And lo and behold, the warning pops up as soon as ffe-radio-button-react is part of the codebase:
https://codesandbox.io/s/641yrknpvz

@ivarni
Copy link
Contributor Author

ivarni commented Nov 23, 2018

@selbekk the problem seems to be that ffe-radio-button-react packs its own version of ffe-form-react and for this warning to not give false positives that version needs to match my own.

I'm going to go ahead and propose moving the dependency to a peerDependency, whatcha think?

EDIT: Better yet, just merge ffe-radio-button-react into ffe-form-react where it arguably already belongs since it relies on CSS from ffe-form.

@selbekk
Copy link
Contributor

selbekk commented Nov 26, 2018

+1 on deprecating ffe-radio-button-react (and ffe-checkbox-react, while you're at it) in favor of pulling those two packages into ffe-form-react. The upgrade path should be simple enough.

@selbekk
Copy link
Contributor

selbekk commented Nov 26, 2018

Great job on figuring out what the bug was!

@ivarni ivarni self-assigned this Nov 28, 2018
@ivarni
Copy link
Contributor Author

ivarni commented Nov 28, 2018

I'll do this when I get around to it, hopefully before Yule.

@ivarni
Copy link
Contributor Author

ivarni commented Dec 7, 2018

Fixed in #530 and #531

@ivarni ivarni closed this as completed Dec 7, 2018
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

No branches or pull requests

2 participants