-
-
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
[Autocomplete] Move useAutocomplete to the Unstyled package #27485
[Autocomplete] Move useAutocomplete to the Unstyled package #27485
Conversation
Details of bundle changes (experimental) @material-ui/lab: parsed: -0.00% 😍, gzip: +0.35% |
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.
It seems that git is considering the moved filed as "new", not "moved". It sounds like we will lose the ability to blame. Could you double-check this aspect and look into keeping the history?
82e8239
to
6965645
Compare
@oliviertassinari even though I moved the files from core to unstyled in one commit (which git correctly recognizes as a move) and created new ones in core in another, the whole diff between this branch and next does not show file movements. |
The last time I faced this problem I have struggled to figure out who (git, github, github PR view, me) was doing it wrong or right. I recall Sebastian had it right in the past. |
Contrary to popular believe git's internals have no notion of "moving" a file. Moving is only computed after the fact (e.g. git-diff, git-blame). You can configure what is considered a "move" in
So it seems like the default threshold is exceeded because we mix moving and changing in a PR. I would recommend we do one commit where we move and another where we change things i.e. two different PRs. |
For git these will be just two commits, so it'll essentially be the same as currently on this branch - even though I made one commit with movement and the other one with editing, when diffing between the latter and the base, git does not recognize the movement (and it's not just a matter of GitHub's PR interface - commandline git shows the same result). |
That's what I said, yes. This PR is just one commit. Splitting this PR would work as the commit that just moved the files will actually mark the files as moved in the diff: |
6965645
to
99fd646
Compare
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.
Looks good. GitHub blame should be able to pick up the move.
Ohh, thanks for the explanation Sebastian, it makes a lot of sense. |
I'm merging to make the review on part two easier #27524 |
One chunk of https://github.com/mui-org/material-ui/issues/27170
This change moves the useAutocomplete hook to the Unstyled package. The hook is reexported from the old location, so nothing should break.
I renamed the
Value
type in useAutocomplete.d.ts toAutocompleteValue
, as this type is specific to the Autocomplete component. The type exported from Core has the oldValue
name to avoid breaking changes.