Skip to content

remove the ui/app and ui/lib folders#10911

Merged
brad-decker merged 2 commits intodevelopfrom
flatten-ui-folder-one-level
Apr 28, 2021
Merged

remove the ui/app and ui/lib folders#10911
brad-decker merged 2 commits intodevelopfrom
flatten-ui-folder-one-level

Conversation

@brad-decker
Copy link
Contributor

@brad-decker brad-decker commented Apr 22, 2021

Problem statement

Giving instructions to new engineers (or even those who have been around the block) to go to the ui/app/components/app folder is somewhat confusing. This would be fine to just overcome, but it also provides no real value. In fact it disjoints the file structure by implying significance of ui/libs over things like ui/app/helpers and to further complicate the matter the ui/libs folder had test helpers in it (some were unused)

Solution

Relocate all files in ui/lib to the most sensible destination, for most this was ui/app/helpers/utils. Once done, flatten the contents of ui/app into ui/ making ui/ the top level folder of the entire frontend portion of the extension.

Also, 1316 files later, I'm sorry.

@brad-decker brad-decker force-pushed the flatten-ui-folder-one-level branch from ef2b786 to e89996a Compare April 22, 2021 16:07
@metamaskbot
Copy link
Collaborator

Builds ready [e89996a]
Page Load Metrics (563 ± 43 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint45715784
domContentLoaded3236555629043
load3256565639043
domInteractive3236545629043

@brad-decker brad-decker marked this pull request as ready for review April 22, 2021 16:26
@brad-decker brad-decker requested review from a team and kumavis as code owners April 22, 2021 16:26
@brad-decker brad-decker requested a review from darkwing April 22, 2021 16:26
@brad-decker brad-decker force-pushed the flatten-ui-folder-one-level branch from e89996a to 4b5a49d Compare April 23, 2021 15:44
@metamaskbot
Copy link
Collaborator

Builds ready [4b5a49d]
Page Load Metrics (655 ± 60 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint518963115
domContentLoaded33884365412560
load33984565512560
domInteractive33884365412560

@darkwing
Copy link
Contributor

Is there a way we can do this without losing all history 😬😬😬 ? Also, it might be good to separate the actual file moves into one commit and the config changes in another, so that rebases etc. aren't a horrible experience.

@brad-decker brad-decker force-pushed the flatten-ui-folder-one-level branch 2 times, most recently from 362f7d9 to b85c8e6 Compare April 26, 2021 19:53
@brad-decker
Copy link
Contributor Author

@darkwing I don't believe preserving history at the file level to be possible in all cases, but we will have the ability to see the file changes and connect the history manually. As an example the first entry for any "new" file in this PR will be the commit in this PR, which if clicked will inform the viewer of the old location. Browsing the repo at that point in history will give the rich file history while it was located in the previous place.

Not ideal, but unfortunately obscuring file history in this way isn't avoidable in many cases.

I did split out the configuration changes into a separate commit

@metamaskbot
Copy link
Collaborator

Builds ready [b85c8e6]
Page Load Metrics (694 ± 40 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint508668115
domContentLoaded4557876938340
load4577876948340
domInteractive4557866938340

@brad-decker brad-decker force-pushed the flatten-ui-folder-one-level branch from b85c8e6 to da07ac6 Compare April 27, 2021 20:47
@metamaskbot
Copy link
Collaborator

Builds ready [da07ac6]
Page Load Metrics (615 ± 31 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint46706084
domContentLoaded4196766146531
load4206776156531
domInteractive4186756146531

darkwing
darkwing previously approved these changes Apr 27, 2021
@brad-decker brad-decker added the DO-NOT-MERGE Pull requests that should not be merged label Apr 27, 2021
@brad-decker
Copy link
Contributor Author

Waiting for the What's new feature to land before merging

@kumavis
Copy link
Member

kumavis commented Apr 27, 2021

@brad-decker why do you think these are not being recognized as file renames?

@kumavis
Copy link
Member

kumavis commented Apr 27, 2021

reading suggestions to prefer git mv when possible

@kumavis
Copy link
Member

kumavis commented Apr 28, 2021

ohhh

git commit never detects renames. It just writes content to the repository. Renames (and copies as well) are only detected after the fact, i.e. when running git diff, git merge and friends. That's because git does not store any rename/copy information.

https://stackoverflow.com/questions/14832963/how-can-i-control-the-rename-threshold-when-committing-files-under-git

is that true? seems like git blame etc would have to re-run rename detection for every commit where the file was added
Edit: apparently it does

@brad-decker
Copy link
Contributor Author

@kumavis Thanks for the lesson! I didn't know the entire interface is just being deciphered! I won't complain about github loading slowly on my huge PRs anymore lol.

@brad-decker brad-decker force-pushed the flatten-ui-folder-one-level branch from 4c2002f to fe530f8 Compare April 28, 2021 18:05
@brad-decker brad-decker removed the DO-NOT-MERGE Pull requests that should not be merged label Apr 28, 2021
Copy link
Contributor

@ryanml ryanml left a comment

Choose a reason for hiding this comment

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

great work!

@brad-decker brad-decker merged commit 09d81ac into develop Apr 28, 2021
@brad-decker brad-decker deleted the flatten-ui-folder-one-level branch April 28, 2021 19:54
@github-actions github-actions bot locked and limited conversation to collaborators Apr 28, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants