Skip to content

Fix extra FormTab/Tab props are passed to two different components - #2654

Merged
fzaninotto merged 4 commits into
masterfrom
unknown repository
Dec 13, 2018
Merged

fzaninotto merged 4 commits into
masterfrom
unknown repository

Conversation

@ghost

@ghost ghost commented Dec 12, 2018

Copy link
Copy Markdown

For issue #2651.

I looked at every component that used Link and decided that Tab and FormTab were the only components where this ability really made sense.

I briefly considered adding docs for this prop, but Tab and FormTab are not documented and neither are their properties, so I thought it best to leave that for a different pull request.

Thank you.

@fzaninotto

Copy link
Copy Markdown
Member

Wait, it's not the right fix. The problem is that FormTab passes extra props both to the tab header and to the tab inputs. That can never work, because there is no prop that can work on such different components.

The right fix is to remove the {...sanitizeRestProps(rest)} in the second part to let you pass any extra prop (like replace) to the root element only.

@ghost

ghost commented Dec 13, 2018

Copy link
Copy Markdown
Author

The right fix is to remove the {...sanitizeRestProps(rest)} in the second part...

So I think what you're saying is to undo all of the other changes and simply remove {...sanitizeRestProps(rest)} from the FormTab renderContent block on this line, allowing all of the previously sanitized props (label, icon, value, translate) to be passed down to each child of FormTab? (Same for detail/Tab.js renderContent?)

I don't think this will work because sanitizeRestProps is already letting the replace prop through to each child of FormTab/Tab and when that happens there is an error:

Warning: Received true for a non-boolean attribute replace.
If you want to write it to the DOM, pass a string instead: replace="true" or replace={value.toString()}.

@ghost

ghost commented Dec 13, 2018

Copy link
Copy Markdown
Author

Oh wait, you're saying to remove it - so none of those props will be passed. I got it, thanks!

@ghost ghost changed the title Add 'replace' prop for Tab, FormTab to allow their Links to replace history Do not forward props to FormTab/Tab child content. Dec 13, 2018
@ghost

ghost commented Dec 13, 2018

Copy link
Copy Markdown
Author

OK, I removed the forwarding of props in the renderContent block of both FormTab and Tab.

In Tab there were 2 places that it forwarded props (to the Labeled element and the cloneElement).

@fzaninotto fzaninotto changed the title Do not forward props to FormTab/Tab child content. Fix extra FormTab/Tab props are passed to two different components Dec 13, 2018
@fzaninotto fzaninotto added this to the 2.5.2 milestone Dec 13, 2018
@fzaninotto

Copy link
Copy Markdown
Member

You were too aggressive when removing the props you passed to FormInput. See SimpleForm to understand which props need to be passed explicitly.

@fzaninotto fzaninotto removed this from the 2.5.2 milestone Dec 13, 2018
@ghost

ghost commented Dec 13, 2018

Copy link
Copy Markdown
Author

OK, sorry about that - I am still waiting over an hour to get this cypress package installed and I can't run any tests on my own machine. So, I just pushed to let Travis do the tests.

I looked at the code for SimpleForm and I saw what it was passing to FormInput. So, I passed those same props from FormTab. However, running my own project in developer mode, I don't see any of those props (basePath, record or resource) in devtools being passed to my FormTab, so I'm not sure how this is going to forward those props to its children.

@fzaninotto

Copy link
Copy Markdown
Member

you'll have the same problem with Tab, see SimpleShowLayout for an example of what props to pass to children (record, resource, basePath).

@ghost

ghost commented Dec 13, 2018

Copy link
Copy Markdown
Author

OK, I just don't see those props anywhere in devtools on a Tab or a FormTab, so I wasn't sure at all; wanted to run a test. I'll push that change in a minute.

@fzaninotto fzaninotto added this to the 2.5.3 milestone Dec 13, 2018
@ghost

ghost commented Dec 13, 2018

Copy link
Copy Markdown
Author

I already knew what to do before I looked at SimpleShowLayout so I didn't put the props in the same order. Sorry about that.

@ghost

ghost commented Dec 13, 2018

Copy link
Copy Markdown
Author

I'm guessing that the 1 test is failing because of another prop that is not being passed? I'm not sure where the error is coming from. The stack trace is useless. The replace property mentioned in the error has nothing to do with any replace property that I was passing, since I removed all reference to that property from FormTab and Tab. Now, the only place that I find the symbol replace being used in the entire package, is in RichTextField

My original solution passed all tests since it only added the feature and changed nothing else. Now we're working on something else. I could start passing some of the other props that I see in devtools, but it takes too long to run these tests and I already spent over 3 hours on this.

This feature turned into something a lot less trivial and I don't think I can spend anymore time on it. Without this feature, things work in production without any errors. I just wanted to fix an error in my console. Sorry.

@ghost ghost closed this Dec 13, 2018
@fzaninotto

Copy link
Copy Markdown
Member

Thanks for your time. I'll take it from then on and try to finish it.

@fzaninotto fzaninotto reopened this Dec 13, 2018
@fzaninotto
fzaninotto merged commit 483a348 into marmelab:master Dec 13, 2018
@fzaninotto

Copy link
Copy Markdown
Member

Thanks, you were very close to the target!

@ghost

ghost commented Dec 13, 2018

Copy link
Copy Markdown
Author

Thank you so much for taking the time to wrap this up!

I might have known to do that since I had previously removed props from being passed to that Label, doh!

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.

1 participant