Fix upgrade for functional components, upgrade Tabs recursively in order to fix ripple effect

[DirtyHairy]

Hi all!

This PR fixes component upgrades to correctly handle stateless functional components.

In addition Tabs require a recursive update via componentHandler.upgradeElements in order for ripple to work. To this end, this PR introduces a recusive mode of operation in MDLComponent. Together, these two changes fix issues #394 and #415.

Both issues do not show up on the documentation pages. This is because the markdown bootstraps calls upgradeElements on the whole page. This PR removes this call, bringing documentation into line with the actual component behavior.

Cheers -Christian

[codecov-io]

Current coverage is 99.27% (diff: 100%)

Merging #421 into master will decrease coverage by 0.08%

Diff Coverage	File Path
•••••••••• 100%	src/Switch.js
•••••••••• 100%	src/Tabs/Tabs.js
•••••••••• 100%	src/IconToggle.js
•••••••••• 100%	src/Layout/HeaderTabs.js
•••••••••• 100%	src/Layout/Layout.js
•••••••••• 100%	src/Menu.js
•••••••••• 100%	src/Checkbox.js
•••••••••• 100%	src/utils/mdlUpgrade.js
•••••••••• 100%	src/utils/MDLComponent.js
•••••••••• 100%	src/Radio.js

Powered by Codecov. Last update af8c4eb...6ab648b

[tleunen]

Good catch for the SFC upgrade. Initially, I let the component which required upgrade as class, and Tabs didn't need it. But this was before trying to fix it and then I forgot it was only for class component.

I'm glad you fixed it for the SFC. And good catch for the reason why it worked in the documentation, but not when users would use the component.

Thanks again for the investigation and the fix. Before merging, because of the change in the upgrade function, I just want to double check the code locally and see if everything is still ok.

[tleunen]

Hi @DirtyHairy, I've tested the PR against the documentation and I have noticed the tabs and ripple inside the Layout header doesn't work properly. An error is thrown in the MDL Ripple about calling style and classlist on undefined.

But it looks fine for the Tabs component!

[DirtyHairy]

Hi @Tommy! Yes, I noticed the same yesterday. That is a side effect of the upgrade now working - previously, nothing would happen without a global uprade, and now the upgrade works, but is incomplete. I have yet to find out what is going wrong here... A short term solution would be disabling the ripple effect for header tabs until we have a full fix.

[DirtyHairy]

Hi @tleunen !

I have fixed the remaining issues and have updated the PR. I cannot find any glitches in the documentation anymore now.

The issue that caused the errors in the console is a nasty race between MDL component handlers. If MDL is used on a static page, the Layout handler will run before the Ripple handler on the Tabbar. The Layout handler tags the Tabbar for special treatment by the Ripple handler by adding a class. However, due to the order in which lifecycle hooks are called in React, it seems that the Tabbar Ripple handler is called before the Layout handler and, as the tag class is missing, attaches event handlers that will throw later.

I have modified HeaderTabs to explicitly add the tag class, which removes the race. Similar quirks are documented as necessary when creating custom MDL components that use ripple, see i.e. google/material-design-lite#4205

Cheers -Christian

[tleunen]

I have modified HeaderTabs to explicitly add the tag class, which removes the race.

Could you explain this? All the changes you made make sense now that the doc doesn't auto upgrade everything in the page, I'll double check just to make sure ;)

[DirtyHairy]

Could you explain this?

I am explicitly adding the class mdl-js-ripple-effect--ignore-events in JSX if ripple is enabled. Usually, this is dynamically set by the Layout handler before the Ripple handler executes but, due to the order in which react calls the lifecycle hooks, in our case Ripple executes first. Setting the class statically prevents the Ripple handler from erroneously attaching the failing event handlers.

So, I guess 'removing the race' is not a precise way to put what I did, 'evading the consequences' is more like it :smirk:

[DirtyHairy]

Thanks for merging... :smile: