refactor(Components): use a class if there are event handlers

[layershifter]

Functional components that need to be turned into a class (Found via grep: const handle):

• AccordionTitle
• BreadcrumbSection
• Card
• DropdownItem
• Form
• Label
• MenuItem
• SearchResult
• Step

[codecov-io]

Current coverage is 100% (diff: 100%)

Merging #619 into master will not change coverage

@@           master   #619   diff @@
====================================
  Files         119    119          
  Lines        1913   1881    -32   
  Methods         0      0          
  Messages        0      0          
  Branches        0      0          
====================================
- Hits         1913   1881    -32   
  Misses          0      0          
  Partials        0      0          

Powered by Codecov. Last update e010928...583800f

[layershifter]

Please, pay attention there, it looks I open there pandora box.

[levithomason]

This seems correct for now. Eventually, the serializer will not be DOM dependent and this ref will go away. I would suggest just setting it and avoiding the developer indirection however.

ref={c => (this._form = this._form || c)}

[layershifter]

Rating and Accordion has more complicated handlers, any ideas for them?

[jeffcarbs]

Rating is dependent on the index and we're essentially _.partialing the handler. As much as I hate data- props, perhaps we set data-index and use that to derive the index in the handlers? It would allow us to just use the this.handleIconClick and this.handleIconMouseEnter for all icons instead of needing an index-dependent handler for each.

AccordionTitle could behave like MenuItem: take an index prop and pass the index prop along with the event onClick. Then accordion could just use the same `handleTitleClick.

That being said, the Accordion component is a little rough. There's a lot of duplicate functionality in the renderPanels and renderChildren functions. We may just want to hold off addressing the callback issue and handle it in a separate PR with a larger refactor (I think the discussion about "Interacting with child props" in these comments https://github.com/TechnologyAdvice/stardust/issues/604 is particularly relevant here)

[layershifter]

As another hateful solution: we can add class Rating.Icon and pass there index with handlers.

[layershifter]

I'll removed Accordion and Rating from there (we'll done their refactor in separate PRs), so we can merge after review.

[jeffcarbs]

As another hateful solution

😆

This looks good to me but I'll defer to @levithomason for a final review. I'm going to wait to update the custom proptype PR until this gets merged because there are gonna be a ton of conflicts due to renaming/moving stuff.

[jeffcarbs]

Actually, looks like there are a few more that I missed

• Search - onMouseDown={e => e.preventDefault()} that line can actually be removed I think. The analogous updates from this PR were carried over and this line should have been removed.
• Modal - Move ref= function to handleRef callback
• Dropdown
  • Move ref= function in render to handleRef callback
  • Move ref= function in renderSearchInput to handleSearchRef callback
  • Move ref= function in renderSearchSizer to handleSizerRef callback

[layershifter]

Actually, looks like there are a few more that I missed

I copied this to #607.

[levithomason]

This seems correct for now. Eventually, the serializer will not be DOM dependent and this ref will go away. I would suggest just setting it and avoiding the developer indirection however.

ref={c => (this._form = this._form || c)}

[levithomason]

Looks good overall, merging!

Handling Conflicts

Let's try to coordinate larger refactors / moving things around going forward. This way we can avoid big conflicts between PRs. When lots of refactoring is required, I'd like to try to clear any major PRs (such as #618) before embarking. This is partly why I've also requested https://github.com/TechnologyAdvice/stardust/pull/587 to be split up. It is easier to manage and less likely to break stuff if we go in bite sized moves.

Accordion

Yea, this is an ugly component. Partly due to the SUI markup, but it also hasn't been visited in some time. It has some issues as well #559, and some missing features #499. Needs some love for sure.

Rating

Let's avoid using the DOM for reading data. I like the Rating.Icon idea because it allows us to keep the data in the virtual DOM where it should be. Testing is far more sane and it is "the React way".

I don't think we should expose this to the user though. It can probably even live at the top of the Rating file. Then, it can have a proper click/enter handler that can call back with the event and it's props. Since this isn't a public component, we'll be in control of what props we pass it, like the index.

(e, props) Digression :/

Not to derail the convo, but I've considered whether or not we just always callback with props on all handlers: (event, props). We've recently updated to always using an object for the second param any way. I'm not sure there is a downside to standardizing to all props. It would allow users to avoid data- attributes as well by knowing that they'll get back what ever props they passed in. The Label already does this.