billy peng ui extract #822

okwme · 2018-06-13T14:47:21Z

Closes #820

Description: Replaces Ni components w Tm

okwme · 2018-06-14T18:52:59Z

ok so i got voyager to run w the component library by going into node_modules/vue-loader and running yarn. This shouldn't be necessary of course, but might have something to do with this:
vuejs/vue-loader#678
also as can be seen in the circle ci test it's a babel issue not understanding "import". I found a similar babel issue here:
babel/babel-loader#149

okwme · 2018-06-14T18:58:46Z

wait now this branch it's working as expected after rm -r node_modules && yarn
might have been the naming corrections i made on tendermint/ui

circle is still failing of course : \

faboweb · 2018-06-18T17:22:23Z

I guess that Babel is not correctly applied to the imported components.

nylira · 2018-06-19T06:53:19Z

@faboweb On second thought: it would be nice if our unit tests supported ES6 Modules -- it's a far better development cycle with yarn link. Instead of having to build the components package after every edit, we can just save the component and wait for live reload.

nylira · 2018-06-19T08:16:43Z

I spent time trying to fix the tests here and on tendermint/ui, without much progress. The fact that Babel is transitioning to version 7, Vue is transitioning to version 3, and Jest is falling behind [1] is making this extra difficult. There's a complex dependency chain filled with incompatibilities, and CircleCI throws another wrench into the gears.

An option we can look into in the near future is dropping Babel. We can start using ES modules in Electron already, with things like this [2][3]

[1] jestjs/jest#4842
[2] https://gist.github.com/smotaal/f1e6dbb5c0420bfd585874bd29f11c43
[3] https://cli.vuejs.org/guide/browser-compatibility.html#modern-mode

nylira · 2018-06-19T08:41:39Z

I'm getting errors like this with yarn testnet:

         ERROR in ../ui/src/components/TmPageHeader/TmPageHeader.vue
         Module not found: Error: Can't resolve 'vue-loader/node_modules/vue-style-loader' in '/Users/pz/Projects/ui/src/components/TmPageHeader'
          @ ../ui/src/components/TmPageHeader/TmPageHeader.vue 4:2-326
          @ ../ui/src/index.js
          @ ./node_modules/babel-loader/lib!./node_modules/vue-loader/lib/selector.js?type=script&index=0!./app/src/renderer/components/wallet/PageWallet.vue
          @ ./app/src/renderer/components/wallet/PageWallet.vue
          @ ./app/src/renderer/components ^\.\/.*$
          @ ./app/src/renderer/routes.js
          @ ./app/src/renderer/main.js
          @ multi (webpack)-dev-server/client?http://localhost:9080 webpack/hot/dev-server ./app/src/renderer/main.js

         ERROR in ../ui/src/components/TmPart/TmPart.vue
         Module not found: Error: Can't resolve 'vue-loader/node_modules/vue-style-loader' in '/Users/pz/Projects/ui/src/components/TmPart'
          @ ../ui/src/components/TmPart/TmPart.vue 4:2-320
          @ ../ui/src/index.js
          @ ./node_modules/babel-loader/lib!./node_modules/vue-loader/lib/selector.js?type=script&index=0!./app/src/renderer/components/wallet/PageWallet.vue
          @ ./app/src/renderer/components/wallet/PageWallet.vue
          @ ./app/src/renderer/components ^\.\/.*$
          @ ./app/src/renderer/routes.js
          @ ./app/src/renderer/main.js
          @ multi (webpack)-dev-server/client?http://localhost:9080 webpack/hot/dev-server ./app/src/renderer/main.js

UPDATE: These errors because webpack doesn't work properly for yarn link or symlinked modules webpack/webpack#811

nylira · 2018-06-19T08:51:36Z

I used module.exports in the index.js of https://github.com/nylira/vue-field and we've been using vue-field without problems for a long time. I wonder if we can solve a lot of these issues by not ES6 modules in https://github.com/tendermint/ui/blob/develop/src/index.js

faboweb · 2018-06-22T00:49:06Z

app/src/renderer/components/common/NiSessionSignIn.vue

@@ -118,7 +118,7 @@ export default {
 }
 </script>
 <style lang="stylus">
-.ni-form-group
+.tm-tm-form-group


nice catch! 😄

faboweb · 2018-06-22T00:50:22Z

app/src/renderer/components/common/VrToolBar.vue

@@ -0,0 +1,31 @@
+<template lang="pug">


What is the VrToolBar?

Vr = Voyager got it

faboweb · 2018-06-22T00:53:54Z

This looks pretty decent. Awesome work @okwme and @nylira . What is the status of this PR? What is missing?

okwme · 2018-06-22T07:14:22Z

@faboweb still need to improve code coverage, although there's not really any new code besides VrToolBar. Actually maybe removing code damaged the coverage report as the missing governance tests now have a larger effect?

I think we need to figure out a system for deciding what goes into the ui library. There are a lot more common components but they're not being used outside of voyager atm. Should they be included in the library? in the PR? if they stay in voyager should we rename them all to Tm prefix? Vr? I did a search and replace on ni- -> tm- to fix inherited style changes, not sure if there are other implications that need to be considered.

Also still need to get them published on npmjs.com, i'm not sure how that permission works. @nylira you have access right? will future updates always need to go through you or should we automate it somehow? I hate seeing npm packages that are behind their project repos...

nylira · 2018-06-22T08:07:46Z

99% of this PR was due to @okwme awesome efforts. Great job.

I'd suggest renaming Vr modules to Tm just to keep everything under the same namespace. We can keep single-use Tm components in Voyager for now, but as we continue to build front end projects we'll likely reuse them--and that's when we can move them out from this repo.

If it's simple, I propose removing governance components from the coverage report. We're not working on governance yet. (We will be soon, as first version of governance is coming into the SDK).

I have access to deploying new modules to the tendermint npm organization. I can give you access too, @okwme. But you need to enable 2FA on your NPM account first. Let me know. I don't know if we can automate publishing because every publish requires you to enter your OTP. How do we give our CI the OTP without compromising security?

okwme · 2018-06-25T10:19:44Z

renamed Vr as well as the remaining Ni prefixed components
removed governance from the coverage report
merged/resolved everything from develop

@nylira I've added 2FA to my npmjs.com account, my username is okwme. Happy to publish tendermint/tm and edit this pr to pull directly from the npm repo.

As for automating w CI I think you're probably right and it's not worth it for the security concern. Maybe instead we just add a git hook to check for the version number and give a reminder to npm version patch and npm publish?

faboweb · 2018-06-26T10:05:25Z

app/networks/genesis.json

@@ -0,0 +1,1459 @@
+{


Why is there this genesis.json now?

not entirely sure where that came from. I replaced my genesis.json with the one you sent me last week. Does this one not match that one?

The location is odd, as genesis.json usually belongs in a folder for a network.

aha, maybe i had some sloppy drag and drop happening! * removing *

faboweb · 2018-06-26T10:07:58Z

app/src/renderer/components/common/TmToolBar.vue

+    TmToolBar
+  },
+  mounted() {
+    // console.log(TmToolBar)


This used to be called VrToolBar and it extended @tendermint/ui/TmToolBar because all voyager pages have those three slots in common. After the last review from @peng he suggested we name all the components Tm but made the names different withiin the component (tm-tool-bar versus vm-tool-bar) so that they would render without recursion.

faboweb · 2018-06-26T10:10:26Z

test/unit/specs/components/common/Page404.spec.js

+  it("has the expected html structure", async () => {
+    await wrapper.vm.$nextTick()
+    wrapper.update()
+    // console.log(wrapper.vm.$el)


$nextTick is added for the scrollbar plugin

faboweb · 2018-06-26T10:10:46Z

test/unit/specs/components/common/PagePreferences.spec.js

@@ -17,7 +17,9 @@ describe("PagePreferences", () => {
    })
  })

-  it("has the expected html structure if connected", () => {
+  it("has the expected html structure if connected", async () => {
+    await wrapper.vm.$nextTick()


Why do we need this here?

$nextTick is added for the scrollbar plugin

faboweb · 2018-06-26T10:12:00Z

test/unit/specs/components/monitor/PageBlock.spec.js

@@ -40,7 +40,9 @@ describe("PageBlock", () => {
    wrapper.update()
  })

-  it("has the expected html structure", () => {
+  it("has the expected html structure", async () => {
+    await wrapper.vm.$nextTick()


Why do we need this here?

$nextTick is added for the scrollbar plugin

Could you maybe add comments describing this?

In general the timing of component rendering, and the need for a lot of wrapper.updates() occurred after moving the components into an external library that had to be explicitly included into babel during the jest tests.

faboweb · 2018-06-26T12:34:01Z

Awesome! :)

okwme · 2018-06-26T14:39:34Z

e2e tests failed but pass for me locally 😢
trying again...

* quick fix for governance fees * changelog

replace TmBarDiscrete

d4a8b91

okwme requested review from faboweb, NodeGuy and nylira as code owners June 13, 2018 14:47

okwme and others added 6 commits June 13, 2018 19:34

replace vue-button and list-item

92ee58e

use version 0.1.0 of @tendermint/ui

15eb56f

merge

db8b1d6

github ui

81dd3ab

only link @tendermint/ui

fe34da1

added back to package

50cd86f

okwme added 8 commits June 18, 2018 11:05

merged

e3dc9ef

TmDataMsg

5a662e8

TmFieldGroup

8db585f

TmFieldGroup

8f8ede9

TmFormGroup

8c0cbc4

TmFormStruct

22a9932

TmPage & TmPageFooter

e8e8049

TmPart

8b1a664

okwme added 5 commits June 19, 2018 13:24

remove TmTabBar

b113525

VrToolBar to compoensate for vuex state and TmToolBar

7a8386e

ni->tm

6901fd4

rename tests tryjest config

e4ad1a2

jest config

d3c78aa

faboweb reviewed Jun 22, 2018

View reviewed changes

okwme added 6 commits June 25, 2018 11:04

tm-tm

229c462

rename and feedback

1a805a2

remove governance from code coverage

f55ad35

merge and resolve conflicts

3028a2c

merge and resolve conflicts #2

c192ce8

werent saved?

7c80d8e

okwme added 2 commits June 26, 2018 11:45

update packages

b4f31d0

merge conflicts

b0f03d6

okwme changed the title ~~WIP: billy peng ui extract~~ billy peng ui extract Jun 26, 2018

faboweb reviewed Jun 26, 2018

View reviewed changes

okwme and others added 3 commits June 26, 2018 13:47

feedback comments from @Fabo

62f5d24

copy paste

c366bb4

Merge branch 'develop' into billy-peng/820-ui-extract

d11c56e

Merge branch 'develop' into billy-peng/820-ui-extract

84f942e

faboweb approved these changes Jun 26, 2018

View reviewed changes

okwme merged commit be57bde into develop Jun 26, 2018

okwme deleted the billy-peng/820-ui-extract branch June 26, 2018 14:52

faboweb pushed a commit that referenced this pull request Jun 2, 2020

Ana/quick fix for governance fees (#822)

8c3db5d

* quick fix for governance fees * changelog

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

billy peng ui extract #822

billy peng ui extract #822

okwme commented Jun 13, 2018

okwme commented Jun 14, 2018

okwme commented Jun 14, 2018

faboweb commented Jun 18, 2018

nylira commented Jun 19, 2018 •

edited

Loading

nylira commented Jun 19, 2018 •

edited

Loading

nylira commented Jun 19, 2018 •

edited

Loading

nylira commented Jun 19, 2018

faboweb Jun 22, 2018

okwme Jun 22, 2018

faboweb Jun 22, 2018

faboweb Jun 22, 2018

okwme Jun 22, 2018

faboweb commented Jun 22, 2018

okwme commented Jun 22, 2018

nylira commented Jun 22, 2018

okwme commented Jun 25, 2018

faboweb Jun 26, 2018

okwme Jun 26, 2018

faboweb Jun 26, 2018

okwme Jun 26, 2018

faboweb Jun 26, 2018

okwme Jun 26, 2018

faboweb Jun 26, 2018

okwme Jun 26, 2018

faboweb Jun 26, 2018

okwme Jun 26, 2018

faboweb Jun 26, 2018

okwme Jun 26, 2018

faboweb Jun 26, 2018

okwme Jun 26, 2018

faboweb commented Jun 26, 2018

okwme commented Jun 26, 2018

billy peng ui extract #822

billy peng ui extract #822

Conversation

okwme commented Jun 13, 2018

okwme commented Jun 14, 2018

okwme commented Jun 14, 2018

faboweb commented Jun 18, 2018

nylira commented Jun 19, 2018 • edited Loading

nylira commented Jun 19, 2018 • edited Loading

nylira commented Jun 19, 2018 • edited Loading

nylira commented Jun 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

faboweb commented Jun 22, 2018

okwme commented Jun 22, 2018

nylira commented Jun 22, 2018

okwme commented Jun 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

faboweb commented Jun 26, 2018

okwme commented Jun 26, 2018

nylira commented Jun 19, 2018 •

edited

Loading

nylira commented Jun 19, 2018 •

edited

Loading

nylira commented Jun 19, 2018 •

edited

Loading