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

david+fede+jordan/1402-Proposal creation #1522

Merged
merged 32 commits into from
Nov 19, 2018
Merged

david+fede+jordan/1402-Proposal creation #1522

merged 32 commits into from
Nov 19, 2018

Conversation

NodeGuy
Copy link
Contributor

@NodeGuy NodeGuy commented Nov 6, 2018 •
edited by fedekunze
Loading

attach proposal creation to view components

Closes #1402
Fixes #1582

  • Added entries in CHANGELOG.md with issue # and GitHub username
  • Reviewed Files changed in the github PR explorer

@NodeGuy
Copy link
Contributor Author

NodeGuy commented Nov 6, 2018

Blocked by #1517, which occurs when a new proposal is created.

@NodeGuy NodeGuy added blocked ✋ issues blocked by other implementations/issues and removed blocked ✋ issues blocked by other implementations/issues labels Nov 6, 2018
@codecov
Copy link

codecov bot commented Nov 8, 2018 •
edited
Loading

Codecov Report

Merging #1522 into develop will increase coverage by 0.67%.
The diff coverage is 97.82%.

@@             Coverage Diff             @@
##           develop    #1522      +/-   ##
===========================================
+ Coverage    96.74%   97.42%   +0.67%     
===========================================
  Files          100      100              
  Lines         1969     1978       +9     
  Branches        95       96       +1     
===========================================
+ Hits          1905     1927      +22     
+ Misses          54       40      -14     
- Partials        10       11       +1
Impacted Files Coverage Δ
...pp/src/renderer/components/staking/PageStaking.vue 100% <ø> (ø) ⬆️
...p/src/renderer/components/governance/ModalVote.vue 100% <ø> (ø) ⬆️
app/src/renderer/components/wallet/PageWallet.vue 91.42% <ø> (ø) ⬆️
app/src/renderer/components/common/VmToolBar.vue 100% <ø> (ø) ⬆️
app/src/renderer/components/common/TmBalance.vue 100% <ø> (ø) ⬆️
...rc/renderer/components/wallet/PageTransactions.vue 97.14% <ø> (ø) ⬆️
.../src/renderer/components/governance/LiProposal.vue 100% <ø> (ø) ⬆️
app/src/renderer/vuex/modules/delegation.js 100% <ø> (ø) ⬆️
app/src/renderer/routes.js 100% <ø> (ø) ⬆️
...src/renderer/components/common/PagePreferences.vue 100% <100%> (ø) ⬆️
... and 11 more

@NodeGuy NodeGuy force-pushed the david/1402-proposal-creation branch from e7f2a91 to bfcb33e Compare November 8, 2018 22:46
@NodeGuy NodeGuy changed the title WIP: David/1402-Proposal creation David/1402-Proposal creation Nov 8, 2018
jbibla
jbibla previously approved these changes Nov 8, 2018
View reviewed changes
@jbibla
Copy link
Collaborator

jbibla commented Nov 8, 2018

the code looks good - but i'm still getting the invalid bech32 prefix. Expected cosmosaccaddr, Got cosmos 400 error. is this working for you @NodeGuy ?

fedekunze
fedekunze previously requested changes Nov 9, 2018
View reviewed changes
Copy link
Contributor

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

Few comments:

  1. Can't add spaces on the title
  2. Theres 2 back buttons on the header ¯_(ツ)_/¯
  3. Prefix error returns 400 once it's submitted

screen shot 2018-11-09 at 4 25 40 pm

@jbibla
Copy link
Collaborator

jbibla commented Nov 9, 2018

Can't add spaces on the title

this is a tendermint UI issue - he opened a ticket for it 😄

@NodeGuy
Copy link
Contributor Author

NodeGuy commented Nov 9, 2018 •
edited
Loading

Can't add spaces on the title

I reported this here: tendermint/cosmos-ui#82.

Theres 2 back buttons on the header ¯_(ツ)_/¯

Nice catch! Fixed.

Prefix error returns 400 once it's submitted

This used to be caused by #1517 but has been fixed for me. Perhaps try running yarn build:gaia?

@fedekunze
Copy link
Contributor

Just realized that this form is currently missing the initital_deposit field:

https://github.com/cosmos/cosmos-sdk/blob/bb54a0de127e45713f272217f578c0abe53a5b21/x/gov/msgs.go#L16-L22

@fedekunze fedekunze changed the title David/1402-Proposal creation WIP: David/1402-Proposal creation Nov 12, 2018
@fedekunze
Copy link
Contributor

@NodeGuy there are some refactors not related to the scope of the PR (e.g: lcdClient.js). Do you mind if I split the PR for ease of review? I could do it tomorrow if u like

@fedekunze fedekunze mentioned this pull request Nov 13, 2018
Merged
2 tasks
@fedekunze
Copy link
Contributor

I've split the refactors into #1567 @NodeGuy @faboweb

@fedekunze fedekunze mentioned this pull request Nov 13, 2018
Closed
@fedekunze
Copy link
Contributor

as per discussion with @faboweb, we are going to implement modal instead of connecting the existing page

@fedekunze
Copy link
Contributor

@jbibla is going to help with styling

@fedekunze
Copy link
Contributor

fedekunze commented Nov 15, 2018 •
edited
Loading

snapshot

e2e is failing bc the last buttons from the header toolbar are not being displayed

@fedekunze fedekunze mentioned this pull request Nov 16, 2018
Closed
faboweb
faboweb reviewed Nov 16, 2018
View reviewed changes
Copy link
Collaborator

@faboweb faboweb left a comment

Choose a reason for hiding this comment

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

Two thumps up for figuring this out @fedekunze

@fedekunze fedekunze changed the title david+fede+jordan/1402-Proposal creation WIP: david+fede+jordan/1402-Proposal creation Nov 19, 2018
@fedekunze
Copy link
Contributor

fixing some things that were broken due to v0.26

@fedekunze fedekunze changed the title WIP: david+fede+jordan/1402-Proposal creation david+fede+jordan/1402-Proposal creation Nov 19, 2018
faboweb
faboweb reviewed Nov 19, 2018
View reviewed changes
test/e2e/launch.js
@@ -232,6 +232,12 @@ async function writeLogs(app, location) {
async function startApp(app, awaitingSelector = `.tm-session`) {
console.log(`Starting app`)
await app.start()
if (!app.browserWindow) {
console.log(`No browser window`)
return
Copy link
Collaborator

Choose a reason for hiding this comment

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

should this crash?

Copy link
Contributor

Choose a reason for hiding this comment

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

It should. Do you want me to remove those lines ?

@fedekunze fedekunze merged commit c0385c2 into develop Nov 19, 2018
@fedekunze fedekunze deleted the david/1402-proposal-creation branch November 19, 2018 11:48
This was referenced Jan 16, 2019
Merged
Merged
Merged
Merged
Merged
This was referenced Feb 14, 2019
Closed
Merged
@faboweb faboweb mentioned this pull request Apr 3, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jbibla jbibla jbibla left review comments

@fedekunze fedekunze fedekunze left review comments

@faboweb faboweb faboweb approved these changes

@nylira nylira Awaiting requested review from nylira

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

Successfully merging this pull request may close these issues.
4 participants
@NodeGuy @jbibla @fedekunze @faboweb