Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MenuList] Convert to a function component #14865

Merged
merged 18 commits into from
Mar 21, 2019

Conversation

ryancogswell
Copy link
Contributor

  • Provide access to MenuList internals via actions property

  • Changed Menu to interact with MenuList only via exposed actions

  • Adjusted Menu and MenuList tests accordingly

No intended changes in behavior. This is prep work for #8191 and a follow-up to #14757.

* Provide access to MenuList internals via actions property

* Changed Menu to interact with MenuList only via exposed actions

* Adjusted Menu and MenuList tests accordingly
@mui-pr-bot
Copy link

mui-pr-bot commented Mar 13, 2019

@material-ui/core: parsed: -0.13% 😍, gzip: +0.06%

Details of bundle changes.

Comparing: 395d925...defd2d3

bundle parsed diff gzip diff prev parsed current parsed prev gzip current gzip
@material-ui/core -0.13% +0.06% 🔺 357,618 357,144 91,093 91,149
@material-ui/core/Paper 0.00% +0.01% 🔺 68,634 68,635 19,979 19,980
@material-ui/core/Paper.esm 0.00% 0.00% 62,366 62,367 19,060 19,060
@material-ui/core/Popper 0.00% -0.01% 30,454 30,454 10,525 10,524
@material-ui/core/styles/createMuiTheme 0.00% 0.00% 17,390 17,390 5,730 5,730
@material-ui/core/useMediaQuery -0.08% -0.10% 2,471 2,469 1,042 1,041
@material-ui/lab 0.00% -0.00% 152,165 152,165 44,548 44,546
@material-ui/styles 0.00% -0.04% 53,839 53,841 15,566 15,560
@material-ui/system -0.01% 0.00% 17,143 17,141 4,522 4,522
Button 0.00% 0.00% 90,924 90,924 26,941 26,941
Modal 0.00% 0.00% 84,711 84,711 25,208 25,209
colorManipulator 0.00% 0.00% 3,234 3,234 1,301 1,301
docs.landing 0.00% 0.00% 51,843 51,843 11,349 11,349
docs.main -0.08% -0.01% 650,044 649,556 201,596 201,578
packages/material-ui/build/umd/material-ui.production.min.js -0.13% +0.01% 🔺 305,881 305,474 83,876 83,883

Generated by 🚫 dangerJS against defd2d3

@ryancogswell
Copy link
Contributor Author

I've seen the errors from the checks and will make those adjustments sometime tomorrow. Let me know if there are any code style changes I should make at the same time.

Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this a lot. Makes it very obvious what methods are public by never exposing any other instance variable and makes testing easier at the same time.

@@ -6,66 +6,95 @@ import ReactDOM from 'react-dom';
import warning from 'warning';
import ownerDocument from '../utils/ownerDocument';
import List from '../List';
import getScrollbarSize from 'dom-helpers/util/scrollbarSize';

function MenuList({ actions, children, className, onBlur, onKeyDown, disableListWrap, ...other }) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We destructure in the function body. Just helps with formatting and many props.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eps1lon and @oliviertassinari -- I'll of course adhere to the standards agreed on by the core team, but I'm curious what the formatting issues are with many props. Hoping to see the formatting ugliness that happens, I switched List.js to destructure props in the function declaration. After running prettier, it looked nearly identical to the previous version -- just without the const at the beginning or the = props; at the end. The indentation was completely identical. My personal preference is to destructure in the function declaration to avoid props being available as an alternate way to access the same variables. I also think it would be a benefit to see the defaults at the declaration of the variable rather than scrolling down to defaultProps. When the defaults are handled via plain JavaScript features, my IDE is also smarter about being able to instantly show me the default for the variable (a keyboard shortcut at any point of usage will show me the declaration). I'm just having a hard time seeing any downsides to destructuring in the function declaration.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm personally using destructuring as few as possible as a general rule. I see two advantages. Number one, it's easier to grep. I can delete code more easily as I have more grep surface. The ability to delete code is one of my top concern when writing it. However, it's less efficient with React components. Advantage number two, in a JavaScript codebase, without TypeScript or JSDoc, you maintain the semantics of the argument, otherwise, it's implicit. However, it's less a concern in React components as it's standardized.
Regarding the defaultProps API. React might get ride of it reactjs/rfcs#107. There is an issue with react-docgen for the generation of the Markdown. @eps1lon has more context than me on it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm don't have any strong preference either way. If we change the approach, my concern n°1 is consistency: do it one way everywhere, not a mix on a migration we never complete.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Number one, it's easier to grep.

I initially agreed with that but with modern JS /function [A-Z]/ should be sufficient. I think this works just as well. For arrow function components const [A-Z].* = ( should do.

you maintain the semantics of the argument

In general yes but for function components the argument semantics are pre-determined. There won't be a function component that starts with context or ref. Always props.

The reasons why I'm arguing for this is

My personal preference is to destructure in the function declaration to avoid props being available as an alternate way to access the same variables.

Especially in a hooks world the immediate destructuring won't even allow accessing props.someProp in a hook. No need for an angry linter to yell at you. Second concern why destructuring should be preferred: Minification. Minification of object properties is pretty unstable as far as I know and conservative minifaction won't touch them. However destructuring helps minification:

// with destructuring
-const { somePrettyLongName } = props;
+const { somePrettyLongName: a } = props;
-const isItReallyLong = somePrettyLongName.length > 10;
+const b = a.length > 10;
-return <div className={somePrettyLongName} />;
+return <div className={a} />;
// without
const isItReallyLong = props.somePrettyLongName.length > 10;
return <div className={props.somePrettyLongName} />;

I'm OK with being inconsistent with argument destructuring here since function components aren't supposed to be treated as functions anyway. Calling them directly is rather rare.

If React follows through with the deprecation of defaultProps we will have to adapt anyway. Enabling react-docgen to parse the function body for default props is probably trivial but it's time spent nonetheless.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eps1lon 👍

packages/material-ui/src/MenuList/MenuList.js Show resolved Hide resolved
packages/material-ui/src/MenuList/MenuList.js Outdated Show resolved Hide resolved
packages/material-ui/src/MenuList/MenuList.js Outdated Show resolved Hide resolved
packages/material-ui/src/MenuList/MenuList.test.js Outdated Show resolved Hide resolved
packages/material-ui/src/MenuList/MenuList.test.js Outdated Show resolved Hide resolved
packages/material-ui/src/MenuList/MenuList.js Show resolved Hide resolved
packages/material-ui/src/MenuList/MenuList.js Outdated Show resolved Hide resolved
);

return React.cloneElement(child, {
tabIndex: index === currentTabIndex ? 0 : -1,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to change the tab index? Could we keep -1 (menu), 0 (dropdown) or nothing (comboxbox) depending on the mode? If we might be able to can kill the currentTabIndex state.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I intend on tackling that next, but I want to take some time to layout all the different scenarios related to tab index in a separate issue and make sure I know what we want the behavior to be in each. It isn't clear to me yet whether or not the dropdown case can be that simple.

packages/material-ui/src/MenuList/MenuList.js Show resolved Hide resolved
…scrollbar size

* scrollbar size is 0px with jsdom, but various sizes in Karma tests with different browsers
* Moved resetTabIndex out of the component so it doesn't need to be an effect dependency

* Removed no-longer-used (due to moving functionality to MenuList) imports from Menu
* Deconstruct props in function body

* Remove redundant findDOMNode calls

* Document StrictMode status of findDOMNode calls
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let us know if you need any help to move the pull request forward :)

packages/material-ui/src/MenuList/MenuList.js Outdated Show resolved Hide resolved
@ryancogswell
Copy link
Contributor Author

@eps1lon I believe all of your review points have been addressed. Let me know if any further changes are necessary.

@eps1lon eps1lon self-requested a review March 18, 2019 13:29
@ryancogswell
Copy link
Contributor Author

On a side note, I'm seeing intermittent Karma test errors in animate.test.js. It happened once in CI and I saw it once locally when executing the browserstack Karma tests.

Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On a side note, I'm seeing intermittent Karma test errors in animate.test.js. It happened once in CI and I saw it once locally when executing the browserstack Karma tests.

I have noticed it too, it's annoying. The test fail doesn't give us much information.

  • Is the logic broken on Safari 10? I have tried the module on the Tabs demo page, it seems to work, plus it's flaky. So no.
  • Does it return an error? We don't check the first argument. We could add an assertion
  • Is this causing by a test leak?

@ryancogswell
Copy link
Contributor Author

I have noticed it too, it's annoying. The test fail doesn't give us much information.

@oliviertassinari Do you know how long ago you first noticed it?

@eps1lon
Copy link
Member

eps1lon commented Mar 18, 2019

I have noticed it too, it's annoying. The test fail doesn't give us much information.

@oliviertassinari Do you know how long ago you first noticed it?

We recently switched from node 6 to 8 in CI. Might be some test that hits timing issues.

@oliviertassinari
Copy link
Member

@ryancogswell I have always seen our karma tests flaky. The SwipeableDrawer fails from time to time too.

packages/material-ui/src/Menu/Menu.test.js Outdated Show resolved Hide resolved
packages/material-ui/src/Menu/Menu.test.js Outdated Show resolved Hide resolved
packages/material-ui/src/MenuList/MenuList.js Outdated Show resolved Hide resolved
},
}));

React.useLayoutEffect(() => {
Copy link
Member

@eps1lon eps1lon Mar 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might need a different strategy here. We can't use any useLayoutEffect for our components since this will disqualify them trigger a warning for server-side rendering with the current version of react. See facebook/react#14927. Might be a legitimate use case for useLayoutEffect in SSR though.

I feel like the individual components shouldn't hold the selected state but the MenuList. As far as I can tell currentTabIndex and components with selected currently fight for the state ownership which might come with a whole host of problems in ConcurrentMode.

Maybe we can use #14585 and only handle imperative focus here. Still leaves the issue of useLayoutEffect on the server. Presenting useLayoutEffect as a focus manager is much more presentable IMO than focus manager + finding children that match a predicate.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My next chunk of work will involve evaluating the use of currentTabIndex in detail and changing the approach. I would prefer to not change that notably in this pull request.

I think changing this useLayoutEffect to useEffect is doable. The main case to make sure I'm handling robustly is the auto-focus case for Menu -- I need to ensure that currentTabIndex is correct when the focus logic executes. This may require a ref for knowing whether or not we have already executed resetTabIndex for mount and if focus gets called first then go ahead and execute resetTabIndex (and then skip doing it in the effect).

Of course, assuming Menu eventually gets converted to a function component, the useLayoutEffect question will come up again since that would be the most natural way to convert its componentDidMount which executes the auto-focus logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After looking at this more closely, I realized this was completely harmless to change from useLayoutEffect to useEffect. The tab index is not used for the auto-focus functionality, so it only matters once the user starts hitting the tab key, and for that purpose it is perfectly fine to wait for useEffect.

@ryancogswell
Copy link
Contributor Author

The additional items @eps1lon mentioned in the follow-up review have now been addressed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change new feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants