Page MenuHomePhabricator

[web] Add other <button> props to Button.
ClosedPublic

Authored by przemek on Dec 6 2022, 2:28 AM.
Tags
None
Referenced Files
F1689720: D5819.diff
Wed, May 1, 5:13 PM
Unknown Object (File)
Tue, Apr 9, 2:42 AM
Unknown Object (File)
Tue, Apr 9, 2:42 AM
Unknown Object (File)
Tue, Apr 9, 2:42 AM
Unknown Object (File)
Tue, Apr 9, 2:41 AM
Unknown Object (File)
Tue, Apr 9, 2:41 AM
Unknown Object (File)
Tue, Apr 9, 2:41 AM
Unknown Object (File)
Wed, Apr 3, 5:53 PM

Details

Summary

I need onMouseMove event in Button, but I'd like to use our generic Button component.

Test Plan

Tested if buttons work.
New functionality tested in next diffs, where I used event.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

Changed mouseOver to mouseMove so it triggers correctly.
It begs a questions whether I should even try to use generic Component and not plain html tag, if use case is so custom.

przemek retitled this revision from [web] Add onMouseOver to Button. to [web] Add onMouseMove to Button..Dec 6 2022, 3:56 AM
przemek edited the summary of this revision. (Show Details)
przemek edited the test plan for this revision. (Show Details)
ashoat requested changes to this revision.Dec 6 2022, 7:05 AM
ashoat added inline comments.
web/components/button.react.js
38 ↗(On Diff #19170)

Wonder if we should just allow for any props that <button> can take, like this

This revision now requires changes to proceed.Dec 6 2022, 7:05 AM
przemek added inline comments.
web/components/button.react.js
38 ↗(On Diff #19170)

Yeah, I agree, I was surprised that props were limited in such way. Should I do it in this diff, or create issue for that and continue in another diff?

przemek marked an inline comment as done.

As @ashoat suggested, I added all <button> props.

web/components/button.react.js
37–43

I've only left things that have default values, or are later explicitly needed in component for further processing.
Everything else is passed straight to the <button> below

tomek added inline comments.
web/components/button.react.js
41

We're no longer setting the default value. Is it intentional? Do we still need to destructure this prop from props or maybe it is better to keep it inside buttonProps?

46–72

It wasn't introduced in this diff, but it's better to use classNames when composing the classes

Awesome, thanks for making that change!! Please address @tomek's comments before landing

This revision is now accepted and ready to land.Dec 8 2022, 10:27 AM
przemek marked an inline comment as done.

Using classNames.

web/components/button.react.js
41

We need it as they are used below to create wrappedChildren:

const wrappedChildren = React.Children.map(children, child => {
    if (typeof child === 'string' || typeof child === 'number') {
      return <span>{child}</span>;
    }
    return child;
  });

Is there a way to access them if they are destructured here?

Sorry, morning brain fart.
Yeah, disabled doesn't need to be there.

przemek retitled this revision from [web] Add onMouseMove to Button. to [web] Add other <button> props to Button..