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

Initial import of new UI (compiled JS code) #2220

Merged
merged 12 commits into from
Oct 17, 2016
Merged

Initial import of new UI (compiled JS code) #2220

merged 12 commits into from
Oct 17, 2016

Conversation

tomusdrw
Copy link
Collaborator

@tomusdrw tomusdrw commented Sep 21, 2016

Signer on http://127.0.0.1:8180 replaced with new UI.
New Dapps are also accessible, served from http://localhost:8080 though.

Old Dapps still included, but should be removed in near future.

parity-dapps-glue is just a copy of parity-dapps in parity-ui repository. When we get rid of old dapps it will be renamed to parity-dapps.

Related: #2155

@tomusdrw tomusdrw added the A0-pleasereview 🤓 Pull request needs code review. label Sep 21, 2016
@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 85.29% when pulling e26ae7f on signer-dapps into 93f82a1 on master.

@tomusdrw tomusdrw added M4-core ⛓ Core client code / Rust. M-ui labels Sep 21, 2016
@jacogr
Copy link
Contributor

jacogr commented Sep 21, 2016

I think it does show us the work still outstanding -

  • compilation of the code and making it accessible to the build process (at this point as soon as integrated, it will fall behind unless manually updated)
  • actual dapps to be served from a different location when integrated (so they don't go via the ws interface)

I'm wondering if this should be merged or rather just kept open until we can address the above 2 points.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 85.288% when pulling 542c0dd on signer-dapps into 93f82a1 on master.

@tomusdrw
Copy link
Collaborator Author

tomusdrw commented Sep 21, 2016

  1. Yes - that's the tough one. We can utilize existing "compile JS as part of rust build" on CI + "use precompiled js" by default. Where does the pre-compiled JS comes from - that's a separate issue.
  2. That one is addressed partially. Dapps are served from 8080, although the address is hardcoded currently.

My proposal is:

  1. 8080 (dapps) - serving built-in dapps
  2. 8081 (dapps+1) - serving "unsafe" dapps
  3. 8180/8546 - WS interface (currently only Signer, in future Pub/Sub + Signer)

@jacogr
Copy link
Contributor

jacogr commented Sep 21, 2016

The build process is easy to integrate, we already have thing done part-way from a GitLab perspective with running lint & test, next step is just running npm run build and pushing the results.

It is just which location to push this to from CI that is the issue. My suggestion - having a js-compiled branch (in the same vein as gh-pages) or even a separate ethcore/js-compiled repo that we can push to from CI. Whichever makes most sense and is easiest/best from a Rust integration perspective. (Obviously from a JS perspective we want to publish to npm as well in this build script, but that is a separate issue, not affecting the binary builds which is primary concern at this point.)

I would like to hear @gavofyork 's views on this as well, if we can reach agreement on destination the implementation of this "build & make available" will be quick.

I have no issues with your locations suggestions (we have discussed it).

@gavofyork
Copy link
Contributor

yeah - happy with @jacogr 's suggestion - pushing precompiled stuff to a separate repo, CI process to build it. will need to work with @General-Beck on this, i guess.

@General-Beck
Copy link
Contributor

Yes, we're working on it

@jacogr
Copy link
Contributor

jacogr commented Oct 12, 2016

We do the have js-precompiled repo operational now. It builds and pushes from GitLab to the branch being built, i.e. js branch will push to js, master to master, beta to beta, etc. (Currently since we treat js as the master for js changes, it is the most up-to-date)

So we can adapt to instead of including the built files in here, pull it from there.

None
#[derive(Default)]
pub struct Handler {
}
Copy link
Contributor

@rphmeier rphmeier Oct 12, 2016

Choose a reason for hiding this comment

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

pub struct Handler;

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 86.39% when pulling 8ff80b5 on signer-dapps into 4bcc9e3 on master.

@jacogr
Copy link
Contributor

jacogr commented Oct 13, 2016

Closes https://github.com/ethcore/parity/issues/2155 (Related UI source-code PRs are https://github.com/ethcore/parity/issues/2607 & https://github.com/ethcore/parity/pull/2612.)

Tested this branch locally, it new ui loads at http://127.0.0.1:8180, token images show up as expected (content addressing work), new dapps load as expected, sending works.

In short: functionally happy with the PR, it does what it says on the tin.

(Not adding A7-looksgood, since I'm not 100% qualified for a code review, but from my side, consider it added)

@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) to 86.425% when pulling 96ddf18 on signer-dapps into 4bcc9e3 on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 86.513% when pulling cf1a746 on signer-dapps into 4bcc9e3 on master.

Tomasz Drwięga and others added 2 commits October 15, 2016 01:18
@tomusdrw please note the alterations - no mixing tabs and spaces in the indentation portion and always just one tab per indent.
@GitCop
Copy link

GitCop commented Oct 15, 2016

There were the following issues with your Pull Request

  • Commit: 7c23bb2
    • Your commit message body contains a line that is longer than 72 characters

Guidelines are available at https://github.com/ethcore/parity


This message was auto-generated by https://gitcop.com

mod tests {
#[test]
fn it_works() {
}
Copy link
Contributor

Choose a reason for hiding this comment

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

meant to be empty?

@gavofyork gavofyork added A8-looksgood 🦄 Pull request is reviewed well. A7-looksgoodcantmerge 🙄 Pull request is reviewed well, but cannot be merged due to conflicts. and removed A0-pleasereview 🤓 Pull request needs code review. A8-looksgood 🦄 Pull request is reviewed well. labels Oct 15, 2016
Conflicts:
	Cargo.lock
	ethcore/res/ethereum/tests
	signer/Cargo.toml
	signer/src/lib.rs
	signer/src/ws_server/session.rs
@GitCop
Copy link

GitCop commented Oct 17, 2016

There were the following issues with your Pull Request

  • Commit: 7c23bb2
    • Your commit message body contains a line that is longer than 72 characters

Guidelines are available at https://github.com/ethcore/parity


This message was auto-generated by https://gitcop.com

@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) to 86.7% when pulling d3998a1 on signer-dapps into b5c65e3 on master.

@gavofyork gavofyork added A8-looksgood 🦄 Pull request is reviewed well. and removed A7-looksgoodcantmerge 🙄 Pull request is reviewed well, but cannot be merged due to conflicts. labels Oct 17, 2016
@gavofyork gavofyork merged commit 6c7af57 into master Oct 17, 2016
@gavofyork gavofyork deleted the signer-dapps branch October 17, 2016 09:56
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants