Part of ENG-843
Uses plain variant of Button introduced in a previous diff for most of the html <button>s and <div>s.
Details
Details
- Reviewers
tomek atul • abosh ginsu - Group Reviewers
Restricted Owners Package (Owns No Changed Paths) - Commits
- rCOMM23cf6daf84b4: [web] Use plain Button instead of html elements
Check if all changed buttons look and work the same. Check if the changed css classes are used in other places.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Comment Actions
Patched changes locally and all the buttons seem to work as before. good work. I would wait until @atul takes a look first before landing
Comment Actions
Looks close, some more memoization things that need to be addressed.
Check if all changed buttons look and work the same.
Wow that's a lot of buttons to check... I checked a few and they look correct, but I'll take your word on the rest. Please make sure you're fully confident that there are no visual regressions before landing.
I also didn't take a super thorough look at all of the styling changes, so please make sure that you're fully confident in all of them before landing.
| web/apps/app-listing.react.js | ||
|---|---|---|
| 56–59 ↗ | (On Diff #17490) | I'd personally prefer something like the following for symmetry: const iconClasses = classnames({ [css.appListingIconState]: true, [css.iconEnabled]: enabled, [css.iconDisabled]: !enabled, }); |
| web/components/menu-item.react.js | ||
| 23 ↗ | (On Diff #17490) | Was this newline intentional? (Totally fine if it was, just wanted to make sure) |
| 25 ↗ | (On Diff #17490) | We don't want to be defining a function in onClick. We'd want to memoize it using the React.useCallback(...) hook. What happens if we just pass onClick (the MenuItem prop) to onClick (the Button prop)? |
| web/settings/relationship/friend-list-row.react.js | ||
| 26–29 ↗ | (On Diff #17490) | Let's memoize buttonColor using the React.useMemo(...) hook. |
| 47–50 ↗ | (On Diff #17490) | Let's memoize buttonColor using the React.useMemo(...) hook |
Comment Actions
Update.
I tried to be as thorough as I could while testing, I will probably take another look at all the buttons before landing.
| web/components/menu-item.react.js | ||
|---|---|---|
| 23 ↗ | (On Diff #17490) | Yeah, I think it looks a bit nicer |
| 25 ↗ | (On Diff #17490) | I thought it would be a problem because onClick (the MenuItem prop) could be void, while the Button prop only allowed null, but flow doesn't complain if I just pass it directly. Thanks for catching this. |
| web/settings/relationship/friend-list-row.react.js | ||
| 26–29 ↗ | (On Diff #17490) | This color isn't dynamic, so I just moved it outside the component. |
Comment Actions
Looks good to me, please make sure you've taken a thorough look and are confident this diff doesn't lead to any visual regressions