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

Batch small changes #16864

Merged
merged 11 commits into from
Aug 4, 2019
Merged

Conversation

oliviertassinari
Copy link
Member

Let me know if there are any of the commits that you would want to see as an independent one. I have done them all in a single row:

  • ES6 modules f3c2c72: Minor change, googling, it seems that the variant without a space is the norm
  • Shorter alternative to outline: none 191076c: I have discovered the alternative on Josh pull request with the tree view. It is smaller, so why not. https://stackoverflow.com/questions/35648667/outline-none-vs-outline-0
  • [Stepper] Move the customization to the top 42fa4e5. I have added a new customization demo. Looking at the GA events, the customization is in position n°3, in most of the other pages, it's in position n°2. The problem is that we have large sections. I'm trying a different tradeoff. I'm moving the demo at the position n°1. We will see how it impacts the GA events of interactions.
  • [docs] Add J.P. Morgan in the homepage 2dddc24. Simple change
  • [docs] Revert top-level imports 52e43f0. This fixes an inconsistency in the documentation. With hindsight, it was probably a waste of time.
  • Remove createStyles when no theme prop is used 276ba50. The demos are inconsistent, I'm going with the most used approach.
  • [docs] Textfield floating label limitation 693c337. Width of TextField does not take label into account when no value #16847 (comment)
  • [docs] Improve Minimizing Bundle Size docs b3df097. Closes docs: named imports #16803. I hope that the changes are not controversial. I have tried the two Babel plugins, it works great. We can encourage it.
  • [docs] Language negotiation with ZH 14cef2d. Redirect the Chinese users to the Chinese version of the documentation, until they decide that it's not the right version for them.

@@ -11,7 +13,7 @@ import Tooltip from '@material-ui/core/Tooltip';
import NoSsr from '@material-ui/core/NoSsr';
import CssBaseline from '@material-ui/core/CssBaseline';
import MenuIcon from '@material-ui/icons/Menu';
import LanguageIcon from '@material-ui/icons/Language';
import LanguageIcon from '@material-ui/icons/Translate';
Copy link
Member Author

@oliviertassinari oliviertassinari Aug 2, 2019

Choose a reason for hiding this comment

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

Use the same icon as React https://reactjs.org/

Capture d’écran 2019-08-02 à 18 25 53

@@ -39,6 +39,7 @@
"@types/react-window": "^1.7.0",
"@types/styled-components": "4.1.12",
"@zeit/next-typescript": "^1.1.1",
"accept-language": "^3.0.18",
Copy link
Member Author

Choose a reason for hiding this comment

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

@mbrookes Your language negotiation logic is back :)

@oliviertassinari oliviertassinari added the umbrella For grouping multiple issues to provide a holistic view label Aug 2, 2019
@mui-pr-bot
Copy link

mui-pr-bot commented Aug 2, 2019

Details of bundle changes.

Comparing: de0c2fd...d33faab

bundle parsed diff gzip diff prev parsed current parsed prev gzip current gzip
@material-ui/core -0.01% -0.01% 329,716 329,671 90,142 90,137
@material-ui/core/Paper 0.00% 0.00% 68,673 68,673 20,470 20,470
@material-ui/core/Paper.esm 0.00% 0.00% 62,058 62,058 19,209 19,209
@material-ui/core/Popper 0.00% 0.00% 29,185 29,185 10,434 10,434
@material-ui/core/Textarea 0.00% 0.00% 5,759 5,759 2,368 2,368
@material-ui/core/TrapFocus 0.00% 0.00% 3,834 3,834 1,617 1,617
@material-ui/core/styles/createMuiTheme 0.00% 0.00% 16,389 16,389 5,824 5,824
@material-ui/core/useMediaQuery 0.00% 0.00% 3,221 3,221 1,312 1,312
@material-ui/lab -0.01% -0.00% 152,604 152,589 46,521 46,520
@material-ui/styles 0.00% 0.00% 51,390 51,390 15,286 15,286
@material-ui/system 0.00% 0.00% 15,753 15,753 4,380 4,380
Button -0.01% -0.00% 79,426 79,421 24,280 24,279
Modal 0.00% 0.00% 15,050 15,050 5,233 5,233
Portal 0.00% 0.00% 3,579 3,579 1,568 1,568
Rating 0.00% 0.00% 70,735 70,735 22,079 22,079
Slider -0.01% -0.00% 75,071 75,066 23,272 23,271
colorManipulator 0.00% 0.00% 3,904 3,904 1,543 1,543
docs.landing +0.08% 🔺 +0.13% 🔺 52,044 52,088 13,816 13,834
docs.main -0.07% -0.12% 590,655 590,258 188,787 188,563
packages/material-ui/build/umd/material-ui.production.min.js -0.02% -0.01% 299,810 299,765 86,177 86,170

Generated by 🚫 dangerJS against d33faab

@oliviertassinari oliviertassinari force-pushed the batch-small-changes-v4 branch 3 times, most recently from e61c197 to 08fb9c1 Compare August 2, 2019 17:10
@mbrookes
Copy link
Member

mbrookes commented Aug 2, 2019

I don't agree with moving the customisation demos to the top, and I don't think that google search ranking should dictate the docs order.

They used to follow a logical flow for someone reading or scanning from top to bottom - basic demo, more advanced demos (or variants), sizes, customisation. Now they are variously a bit of a hodge-podge, depending on how randomly they got rearranged.

If someone's using google the search for component customisation, they won't care that it's at the bottom.

Otherwise, 👍 to the rest of the PR.

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Aug 2, 2019

I'm sorry, I should have made it more clear. I haven't considered the google search ranking. I try to optimize for reducing the aggregated scroll depth. I'm trying to put the demos that trigger more code expand events closer to the top of the page.

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Aug 2, 2019

Does it change your point of view? I agree that the optimization of this metric should be balanced with the top-down logic of the content. It should still make sense.

@mbrookes
Copy link
Member

mbrookes commented Aug 2, 2019

I was looking that the expand events (people looking at the source).

Okay, that makes more sense. I'm still not totally convinced about the reordering that was done some time ago, but live with it. Having the customisation at the top is just wrong though.

For a new user visiting the component pages, they want to see and touch the native component implementation. Customisation should come later. It isn't that hard to find even if it is at the bottom. (Arguably if that were consistent, it makes it very easy!)

@oliviertassinari
Copy link
Member Author

Having the customisation at the top is just wrong though.

I have changed the order, I think that I have found a more logical place for the demo :)

@mbrookes mbrookes merged commit cc21255 into mui:master Aug 4, 2019
@mbrookes
Copy link
Member

mbrookes commented Aug 4, 2019

👌

@oliviertassinari oliviertassinari deleted the batch-small-changes-v4 branch August 5, 2019 04:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
umbrella For grouping multiple issues to provide a holistic view
Projects
None yet
Development

Successfully merging this pull request may close these issues.

docs: named imports
3 participants