Skip to content
This repository has been archived by the owner on Feb 26, 2020. It is now read-only.

Move Dapps files from @parity/shared ; display icons of builtin dapps from filesystem #114

Merged
merged 12 commits into from
May 23, 2018
Merged

Move Dapps files from @parity/shared ; display icons of builtin dapps from filesystem #114

merged 12 commits into from
May 23, 2018

Conversation

axelchalon
Copy link
Contributor

@axelchalon axelchalon commented May 18, 2018 •
edited
Loading

  • Merge dappsViews onto dappsBuiltin
  • Move dapps files from @parity/shared to the shell
  • Fetch icons of builtin dapps from filesystem using manifest.json

This was referenced May 18, 2018
Closed
Merged
Closed
@amaury1093 amaury1093 mentioned this pull request May 21, 2018
Closed
@axelchalon
Update DappsStore tests (defaultDapps)
2983ca4 
They were previously outdated in @parity/shared.

Removed the test "saved views keeps non-specified disabled keys"
because we don't have any dapp to test against in dappsBuiltin.json
(no builtin dapp with a defined id and with visible set to false)
@axelchalon axelchalon changed the title Move Dapps files from @parity/shared Move Dapps files from @parity/shared ; display icons of builtin dapps from filesystem May 22, 2018
@axelchalon axelchalon added the A0-pleasereview Pull request needs code review. label May 22, 2018
@axelchalon axelchalon mentioned this pull request May 22, 2018
Merged
@axelchalon
Copy link
Contributor Author

axelchalon commented May 22, 2018 •
edited
Loading

Build is failing for some strange reason. I don't have the rights to relaunch the build. This being said npm run test fails because of window.require (for fs and electron); not sure how to remedy this.

Uses parity-js/ui#9. Waiting for it to be merged so that I can update the @parity/ui dependency.

@amaury1093
Copy link
Contributor

@axelchalon I sent you an invite for gitlab on [email protected]

@amaury1093
Copy link
Contributor

I have one question: if I use DappIcon inside a dapp (dapp-visible or dapp-methods use it), will this still work?

For the window.require, maybe try if isElectron(), then window.require or else require.

@axelchalon
Copy link
Contributor Author

👍 Thanks for the tip, tests work fine now!

Good question! It seems to be broken inside the dapps at the moment; I'll look into this.

@axelchalon
Copy link
Contributor Author

axelchalon commented May 23, 2018 •
edited
Loading

dapp-visible and dapp-methods should work fine now; a bit tricky to test locally because dev dapps are served through http and don't have access to the fs -- but as far as I tested they work fine. We'll just have to update their @parity/ui dependency.

Edit: and update the dependency to dapp-visible and dapp-methods in the package-lock.json of the shell

@axelchalon
Update @parity/ui dependency
b4ac594
@axelchalon
Update dapp-dapp-methods & dapp-dapp-visible dependencies
6529354 
Had to specify the exact rev in package.json because npm somehow
couldn't properly update the dependencies otherwise:
dapp-dapp-methods would overwrite dapp-dapp-visible in package-lock
and vice versa.
@axelchalon
Copy link
Contributor Author

dapp-visible and dapp-methods work fine here! PR ready for review/merge

amaury1093
amaury1093 reviewed May 23, 2018
View reviewed changes
src/util/dapps.js Outdated
// path.join in Windows would handle everything for us, but after some time
// I realized that even in Windows path.join here bahaves like POSIX (maybe
// it's electron, maybe browser env?). Switching to '/'. -Amaury 12.03.2018
const posixDirName = basePath.replace(/\\/g, '/');
Copy link
Contributor

Choose a reason for hiding this comment

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

This piece of code is now in 2 places. We could add another file inside util/ to factorize that.

amaury1093
amaury1093 approved these changes May 23, 2018
View reviewed changes
Copy link
Contributor

@amaury1093 amaury1093 left a comment

Choose a reason for hiding this comment

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

Yes, works perfectly. I tried with parity --chain dev, all the icons now show.

@amaury1093 amaury1093 added A8-looksgood Pull request is reviewed well. and removed A0-pleasereview Pull request needs code review. labels May 23, 2018
@amaury1093 amaury1093 merged commit 9ff53d3 into master May 23, 2018
@amaury1093 amaury1093 deleted the ac-move-dapps-from-shared branch May 23, 2018 16:39
@axelchalon axelchalon mentioned this pull request May 28, 2018
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@amaury1093 amaury1093 amaury1093 approved these changes

Assignees
No one assigned
Labels
A8-looksgood Pull request is reviewed well.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@axelchalon @amaury1093