-
-
Notifications
You must be signed in to change notification settings - Fork 32.3k
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
[core] Remove dead and redundant code #33125
[core] Remove dead and redundant code #33125
Conversation
f15a3d7
to
302a784
Compare
I'm okay with not moving forward with it 😬 I get that we could use this moment to have a brief explanation of the product but I just feel like it gets too clumsy. Would keep it simple! |
@danilo-leal The main issue I have with the PR's version is that it optimizes for new users. As an existing user, I find it frustrating to have a layout shift/distraction each time I want to quickly go to the docs I care about. It's not clear to me if this is a step forward or backward, I don't know. Did you try this exact pattern in the past or another variation of it? To decide, I would propose that we have more people on the team try the UX and see how they feel about it compared to HEAD. Off topic 20220614-115713-413.mp4 |
I was going to add the same comment, maybe we should increase the delay for closing the popup? |
This reverts commit 302a784.
Alright, It doesn't seem that this got traction. I personally hate that it moves, I guess forcing new developers to click isn't that problematic, we kind of force them to be curious 😁. I have reverted the second commit, so this PR is now a "core" polish change. Maybe the "best" solution would be more about this pattern 🤷♂️:
Or implement the magic triangle 🪄 https://bjk5.com/post/44698559168/breaking-down-amazons-mega-dropdown 😁 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking great to me, as far as the menu appearance speed goes!
import joyPkgJson from '../../../../packages/mui-joy/package.json'; | ||
import basePkgJson from '../../../../packages/mui-base/package.json'; | ||
import systemPkgJson from '../../../../packages/mui-system/package.json'; | ||
import materialPkgJson from 'packages/mui-material/package.json'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't work in MUI X inside @mui/monorepo
package.
I'll submit a PR to revert this as a quick fix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohh, now we have solved the mystery! cc @Janpot
For some reason MUIX setup understands the packages/... alias and I can't figure out why.
It's simply because we were not looking at the right source. MUI X didn't update to the latest version of mui/material-ui HEAD yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that explains a lot 😄.
There are two commits in this PR. The first one is cleanup that we should go in.
The second commitis experimentation. I'm trying point 2. of #32707 (comment) out. At least so we have a reference point that can be checked:Screen.Recording.2022-06-12.at.19.07.57.mov
It feels strange, do we want to move forward with it? Maybe not
https://deploy-preview-33125--material-ui.netlify.app/