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.

Sign up for GitHub

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

Jump to bottom

[docs] Fix Algolia top level duplication #15738

Merged
merged 6 commits into from
May 18, 2019
Merged

[docs] Fix Algolia top level duplication #15738

merged 6 commits into from
May 18, 2019

Conversation

oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented May 18, 2019 •
edited
Loading

  • It should only display "Components", not "Components noise noise noise noise noise etc.":

Capture d’écran 2019-05-18 à 00 25 19

  • Increase CodeFund ad split share to 20% as they seem to perform well, let's see.
  • Fix a focus style issue with a customization demo.
  • Keep track of the bundle size of ClickAwayListener.
    @material-ui/core/ClickAwayListener +Infinity% 🔺 +Infinity% 🔺 0 3,586 0 1,477

@oliviertassinari oliviertassinari added the docs Improvements or additions to the documentation label May 18, 2019
@mui-pr-bot
Copy link

mui-pr-bot commented May 18, 2019 •
edited
Loading

Details of bundle changes.

Comparing: ffffcb2...5592401

bundle parsed diff gzip diff prev parsed current parsed prev gzip current gzip
@material-ui/core 0.00% 0.00% 316,713 316,713 86,546 86,546
@material-ui/core/Paper 0.00% 0.00% 67,869 67,869 20,165 20,165
@material-ui/core/Paper.esm 0.00% 0.00% 61,152 61,152 18,955 18,955
@material-ui/core/Popper 0.00% 0.00% 28,740 28,740 10,354 10,354
@material-ui/core/Textarea 0.00% 0.00% 5,513 5,513 2,383 2,383
@material-ui/core/TrapFocus 0.00% 0.00% 3,744 3,744 1,580 1,580
@material-ui/core/styles/createMuiTheme 0.00% 0.00% 15,960 15,960 5,779 5,779
@material-ui/core/useMediaQuery 0.00% 0.00% 2,106 2,106 974 974
@material-ui/lab 0.00% 0.00% 140,518 140,518 42,850 42,850
@material-ui/styles 0.00% 0.00% 51,353 51,353 15,184 15,184
@material-ui/system 0.00% 0.00% 14,458 14,458 4,175 4,175
Button 0.00% 0.00% 85,781 85,781 25,765 25,765
Modal 0.00% 0.00% 20,344 20,344 6,698 6,698
colorManipulator 0.00% 0.00% 3,904 3,904 1,543 1,543
docs.landing 0.00% 0.00% 47,836 47,836 10,920 10,920
docs.main 0.00% 0.00% 655,909 655,921 206,023 206,025
packages/material-ui/build/umd/material-ui.production.min.js 0.00% 0.00% 295,580 295,580 84,056 84,056

Generated by 🚫 dangerJS against 5592401

@eps1lon
Copy link
Member

eps1lon commented May 18, 2019

Keep track of the bundle size of ClickAwayListener.

Where is the value in that? It's a little helper component that isn't really useful on its own.

@oliviertassinari
Copy link
Member Author

oliviertassinari commented May 18, 2019 •
edited
Loading

Where is the value in that? It's a little helper component that isn't really useful on its own.

@eps1lon It's a good question. I believe these downloads https://npm-stat.com/charts.html?package=react-outside-click-handler stats show that there is a use case for such a component.

In an ideal world, I would document the bundle size of all the modules: https://twitter.com/olivtassinari/status/1023228214762725377.

@eps1lon
Copy link
Member

eps1lon commented May 18, 2019

@eps1lon It's a good question. I believe these downloads npm-stat.com/charts.html?package=react-outside-click-handler stats show that there is a use case for such a component.

I doubt anybody is installing @material-ui/core for a click away handler. I want to avoid making fixs/features any harder because there's a +20% increase in the component when the overall contribution is minimal.

@oliviertassinari
Copy link
Member Author

oliviertassinari commented May 18, 2019 •
edited
Loading

I doubt anybody is installing @material-ui/core for a click away handler.

I hope so, we are the best positioned to provide it.

I want to avoid making fixs/features any harder because there's a +20% increase in the component when the overall contribution is minimal.

Why would it create an overhead? This change is about transparency.

@eps1lon
Copy link
Member

eps1lon commented May 18, 2019

Why would it create an overhead? This change is about transparency.

Well I view the tracking more of a "caution! increases should be re-evaluated". The transparency part is already given with the stats.json build artifacts.

If you just want to add this so that it's easier inspectable we should put this on hold. I have other ideas about inspecting sizes of individual component (sets).

@oliviertassinari
Copy link
Member Author

Ok, I'm revering. https://stackoverflow.com/questions/32553158/detect-click-outside-react-component 150k views, it's a common problem.

@oliviertassinari oliviertassinari merged commit 3b06bdd into mui:next May 18, 2019
@oliviertassinari oliviertassinari deleted the docs-fix-display branch May 18, 2019 20:56
@mbrookes
Copy link
Member

Was this intentional?

image

@oliviertassinari
Copy link
Member Author

oliviertassinari commented May 19, 2019 •
edited
Loading

No 😢

mbrookes
mbrookes reviewed May 19, 2019
View reviewed changes
docs/src/modules/components/AppDrawer.js
const openImmediately = Boolean(
page.subheader || activePage.pathname.indexOf(`${page.pathname}/`) === 0,
);
const topLevel = activePage.pathname.indexOf(`${page.pathname}/`) === 0;
Copy link
Member

Choose a reason for hiding this comment

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

It's here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Should I take care of 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 on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mbrookes mbrookes mbrookes left review comments

Assignees
No one assigned
Labels
docs Improvements or additions to the documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@oliviertassinari @mui-pr-bot @eps1lon @mbrookes