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.

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

Billy/1054 my stake page #1272

Merged
merged 14 commits into from
Sep 5, 2018
Merged

Billy/1054 my stake page #1272

merged 14 commits into from
Sep 5, 2018

Conversation

okwme
Copy link
Contributor

@okwme okwme commented Sep 4, 2018

Closes #1054
with .red, .green and .orange classes added to some of the values it looks like this. I added back the checkbox since as of now there's no other way to get to those pages. The tab is disabled for "My Stake" because that task is now part of #1281, #1278, #1280.

screenshot 2018-09-04 12 38 32

@okwme okwme changed the title Billy/1054 my stake page WIP: Billy/1054 my stake page Sep 4, 2018
@codecov
Copy link

codecov bot commented Sep 4, 2018

Codecov Report

Merging #1272 into develop will decrease coverage by 0.1%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           develop    #1272      +/-   ##
===========================================
- Coverage    95.91%   95.81%   -0.11%     
===========================================
  Files           82       82              
  Lines         1615     1624       +9     
  Branches        76       75       -1     
===========================================
+ Hits          1549     1556       +7     
- Misses          60       61       +1     
- Partials         6        7       +1
Impacted Files Coverage Δ
.../src/renderer/components/staking/PageValidator.vue 100% <ø> (ø) ⬆️
app/src/renderer/components/staking/PanelSort.vue 100% <ø> (ø) ⬆️
app/src/renderer/scripts/common.js 100% <100%> (ø) ⬆️
...pp/src/renderer/components/staking/PageStaking.vue 100% <100%> (ø) ⬆️
app/src/renderer/components/staking/LiDelegate.vue 86.2% <100%> (-5.46%) ⬇️

@okwme
Copy link
Contributor Author

okwme commented Sep 4, 2018

@faboweb and i discussed and it's ok to approve this PR even tho it doesn't pass codecoverage because the only part of LiDelegate not being tested is a part which should actually be removed as per this issue: #1276

@okwme okwme changed the title WIP: Billy/1054 my stake page Billy/1054 my stake page Sep 4, 2018
fedekunze
fedekunze previously approved these changes Sep 4, 2018
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.

LGTM

@@ -1,3 +1,16 @@
module.exports.sleep = function(ms) {
return new Promise(resolve => setTimeout(resolve, ms))
}

module.exports.shortAddress = function(address, length = 4) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe add a comment on what this does

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

display inline-block
margin auto
&:first-of-type .label-text:after
// content ''
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we need to keep those comments?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👎

tasks/runner.js Outdated
@@ -61,6 +61,7 @@ function startRendererServer() {
}

module.exports = async function(networkPath) {
console.log(networkPath)
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we need that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👎

tasks/testnet.js Outdated
@@ -5,7 +5,7 @@ async function main() {
const network = process.argv[2] || config.default_network

// run Voyager in a development environment
runDev(`./app/networks/${network}/`)
runDev(`./builds/testnets/${network}/`)
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this hiding the problem?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🙃

padding 12px 1em
background-color var(--app-nav)

& > .li-delegate__value:not(:first-of-type) span
Copy link
Collaborator

Choose a reason for hiding this comment

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

highly impressive css 👏 🥇

Copy link
Contributor Author

Choose a reason for hiding this comment

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

be wary of :first-of-type it only refers to native elements, not classes!!!

.li-delegate__value.percent_of_vote
span {{ delegate.percent_of_vote }}
.li-delegate__value.uptime
span.NOT_green {{ uptime }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

what is this NOT about?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because we don't have any logic to apply when things are green, orange or red. the classes to enable them are .green .yellow and .red so these are just references to the fact those classes exist but aren't being used yet. (would hate to think of someone in the future re-buiding them when that functionality is finally available!)

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't quite get that. Would you please explain it again in German?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess those are placeholders. I replace them with comments.

tm-data-empty(v-else-if="delegates.delegates.length === 0")
data-empty-search(v-else-if="sortedFilteredEnrichedDelegates.length === 0")
tm-data-loading(v-if="delegates.loading && sortedFilteredEnrichedDelegates.length === 0")
tm-data-empty(v-else-if="!delegates.loading && delegates.delegates.length === 0")
Copy link
Collaborator

Choose a reason for hiding this comment

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

should delegates.delegates.length be sortedFilteredEnrichedDelegates.length?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i switched the logic because loading was getting triggered over and over again when there were accurate validators in the store already. The logic should be that you see the current validator set if there is one, and it gets updated when new info is added, but should only show the loading screen if there are no current validators while you're loading new ones.

Copy link
Collaborator

Choose a reason for hiding this comment

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

smells like a refactor (one day).

delegates.delegates.delegates.delegates.delegates.goose

🦆 🦆 🦆 🦆 🦆

Copy link
Collaborator

Choose a reason for hiding this comment

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

didn't we have the discussion before? ;)

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.

I addressed the comments.

@faboweb faboweb merged commit af262a5 into develop Sep 5, 2018
@faboweb faboweb deleted the billy/1054-my-stake-page branch September 5, 2018 09:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants