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

Folder restructure #6304

Merged
merged 30 commits into from
Mar 21, 2019
Merged

Folder restructure #6304

merged 30 commits into from
Mar 21, 2019

Conversation

chikeichan
Copy link
Contributor

@chikeichan chikeichan commented Mar 14, 2019 •
edited
Loading

Screen Shot 2019-03-14 at 1 50 32 AM

❤️

@chikeichan chikeichan requested a review from danjm March 14, 2019 06:48
@whymarrh
Copy link
Contributor

I know this I should've raised this earlier, but after looking at the image in the OP, can we drop ui/app/? Directories like ui/app/components/app/ and ui/app/ducks/app/ seem a bit redundant. We could instead have ui/components/ and ui/ducks/. Thoughts?

danjm
danjm requested changes Mar 18, 2019
View reviewed changes
Copy link
Contributor

@danjm danjm left a comment

Choose a reason for hiding this comment

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

QA'd and everything seems good.

Inspected the folder structure via my text editor. Its definitely a big improvement.

One preference I have that differs from what is there:

I like having app.js, root.js, reducers.js and index.js in the same place. These pull together all the high level pieces of the app and are a good starting point to understanding the app as a whole. So I think there are benefits for code readability if they are are all together.

Independent of that concern, I would add that app.js and root.js don't really fit in pages/. That directory makes sense as a "directory of all components that have their own route". Whereas app.js is more of a routing component and root.js is boilerplate react needed for the app to work. Perhaps put these one directory up: app/app.js and app/root.js app.js may need a renaming if this is done.

Other than these relatively minor concerns, I think this is good to merge.

@chikeichan
Copy link
Contributor Author

@danjm great suggestions - based on them:

  • changed pages/app.js to pages/routes/index.js
  • changed pages/root.js to pages/index.js
  • move all pages/*.js to pages/[component-name]/index.js
  • move store/reducers.js to ducks/index.js

I actions.js in the store/ directory. This is the file to be re-organize into appropriate ducks.

@chikeichan chikeichan requested a review from danjm March 19, 2019 01:48
danjm
danjm previously approved these changes Mar 20, 2019
View reviewed changes
danjm
danjm previously approved these changes Mar 21, 2019
View reviewed changes
chikeichan added 18 commits March 21, 2019 15:25
@chikeichan
Remove ui/app/keychains/
0b6eaab
@chikeichan
Remove ui/app/img/ (unused images)
2ff1f71
@chikeichan
Move conversion-util to helpers/utils/
43c2532
@chikeichan
Move token-util to helpers/utils/
474d6fd
@chikeichan
Move /helpers/*.js inside /helpers/utils/
4983598
@chikeichan
Move util tests inside /helpers/utils/
7b1592e
@chikeichan
Renameand move confirm-transaction/util.js to helpers/utils/
fa0d258
@chikeichan
Move higher-order-components to helpers/higher-order-components/
6aaa10b
@chikeichan
Move infura-conversion.json to helpers/constants/
d20df84
@chikeichan
Move all utility functions to helpers/utils/
90cce04
@chikeichan
Move pages directory to top-level
4e35fbc
@chikeichan
Move all constants to helpers/constants/
be51b49
@chikeichan
Move metametrics inside helpers/
2cb6cd1
@chikeichan
Move app and root inside pages/
062c766
@chikeichan
Move routes inside helpers/
3b3fa15
@chikeichan
Re-organize ducks/
c4766f4
@chikeichan
Move reducers to ducks/
bd26a0b
@chikeichan
Move selectors inside selectors/
8e6a41a
@danjm danjm merged commit 3117562 into MetaMask:develop Mar 21, 2019
danfinlay pushed a commit that referenced this pull request Mar 26, 2019
@tmashuang @danfinlay
Update master branch (#6296)
b921c8f 
* Fixes the 'Percentages row tracks and gutters' decpracation warning. (#6244)

* Fix missing this reference in addtoken button onclick method (#6245)

* Only call onRecipient and onSender methods if defined in sender-to-recipient (#6247)

* Prevent advanced gas input arrows from setting value to < 0 (#6248)

* Add privacy policy link to modal metrics opt-in (#6250)

* Handle undefined gas limits and prices in transaction-breakdown.component (#6246)

* Uppercase and context fixes on Spanish translation.

* development - enhancement for sourcemap validator tool (#6277)

* Nonmultiple notifications for batch txs

* No longer check network when validating checksum addresses

* Version 6.2.1 (#6251)

* Remove line rather than comment out

* Fixes the use of the browser back button on the reveal seed screen

* GABA: Integrate AddressBookController (#5847)

* gaba: integrate AddressBookController

* pin gaba version and update lockfile

* ci: Use cache version from circle environment var (#6286)

* Allow npm to update the package-lock.json file

* npm i -D [email protected]

* mascara - remove from project (#6283)

* circleci - disable npm dep cache (#6288)

* Centre all notification popups

* ui - add missing PropTypes (#6287)

* Improve Korean translations (#6268)

* ci: Use npm ci for fast(er) installs

* build - babel - move config to babelrc (#6284)

* Patch/bump version (#6294)

* Update Node minor version

* ci: Skip updating npm@6 as it is default

* Allow seed phrases with a trailing newline

* Centre the notification in the current window (#6307)

* Fixes popups not showing when screen size is odd (#6312)

* Fix typos in English messages (#6317)

* Add rollback script, move auto-changelog script (#6252)

* Bump gaba version to avoid broken eth-contract-metadata

* Fixing spelling of Ethereum in MetaMetrics copy (#6329)

* Stop reloading dapps on network change allowing dapps to decide if it should refresh or not (#6330)

* feat: `inpageProvider.autoRefreshOnNetworkChange` to allow dapps to control if it refreshes or not

* feat: check the `autoRefreshOnNetworkChange` before a refresh

* fix linting error

* fix: use `window.ethereum` now `web3.ethereum`

* Enable mobile sync (#6332)

* enable mobile sync

* remove mobile sync as a preference

* Fix typo

* Folder restructure (#6304)

* Remove ui/app/keychains/

* Remove ui/app/img/ (unused images)

* Move conversion-util to helpers/utils/

* Move token-util to helpers/utils/

* Move /helpers/*.js inside /helpers/utils/

* Move util tests inside /helpers/utils/

* Renameand move confirm-transaction/util.js to helpers/utils/

* Move higher-order-components to helpers/higher-order-components/

* Move infura-conversion.json to helpers/constants/

* Move all utility functions to helpers/utils/

* Move pages directory to top-level

* Move all constants to helpers/constants/

* Move metametrics inside helpers/

* Move app and root inside pages/

* Move routes inside helpers/

* Re-organize ducks/

* Move reducers to ducks/

* Move selectors inside selectors/

* Move test out of test folder

* Move action, reducer, store inside store/

* Move ui components inside ui/

* Move UI components inside ui/

* Move connected components inside components/app/

* Move i18n-helper inside helpers/

* Fix unit tests

* Fix unit test

* Move pages components

* Rename routes component

* Move reducers to ducks/index

* Fix bad path in unit test

* Hide gas price chart and prevent api call when not on ethereum networks. (#6300)

Add missing translations in gas customization modal

* Fix gas fee in the submitted step of the transaction details activity log. (#6301)

* Fix oversized loading overlay on gas customization modal. (#6326)

* Replaces the coinbase link in the deposit modal with one for wyre (#6302)

* New settings page rebased (#6333)

* New setting tab

* Add InfoTab

* Add Advanced tab

* Add Security Tab

* Finish mobile view

* Make new setting page responsive

* Fix linter

* Fix y scrolling

* Update link in network dropdown

* Fix e2e tests

* Remove duplicate translation key

* Resolve merge conflict

* Only change settings header in popup view.

* Place mobile-sync button in advanced-tab of settings

* Close transaction on close of notification window (#6340)

* Cancel error rebased (#6341)

* Check balance before showing cancel

* Fix linter

* Use existing helper methods for calculating increased cancel price

* Add tooltip for disabled button

* Lint fix for cancelError branch.

* Disabling of cancel button should account for value of tx.

* E2E - Dont close window notifications (#6349)

* Dont close window notifications

* Remove commented out lines in beta spec

* Don't include tx value in calculation of balance sufficiency for cancel button disabling. (#6346)

* enable privacy mode for first time users (#6347)

* Version 6.3.0 (#6350)
@Gudahtt Gudahtt mentioned this pull request Dec 16, 2020
Closed
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danjm danjm danjm approved these changes

@frankiebee frankiebee Awaiting requested review from frankiebee

@whymarrh whymarrh Awaiting requested review from whymarrh

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@chikeichan @whymarrh @danjm