Page MenuHomePhabricator

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

Authored by przemek on Dec 6 2022, 2:28 AM.
Tags
None
Referenced Files
F3393343: D5819.diff
Sat, Nov 30, 1:17 PM
Unknown Object (File)
Wed, Nov 27, 7:50 PM
Unknown Object (File)
Sun, Nov 24, 11:55 AM
Unknown Object (File)
Sun, Nov 24, 11:55 AM
Unknown Object (File)
Sun, Nov 24, 11:55 AM
Unknown Object (File)
Sun, Nov 24, 11:55 AM
Unknown Object (File)
Fri, Nov 8, 1:02 PM
Unknown Object (File)
Fri, Nov 8, 9:58 AM

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

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

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 ↗(On Diff #19220)

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 ↗(On Diff #19220)

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 ↗(On Diff #19220)

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 ↗(On Diff #19220)

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..