-
Notifications
You must be signed in to change notification settings - Fork 98
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
Jordan/2188 page network bug #2214
Conversation
@@ -87,7 +87,7 @@ export default { | |||
default: undefined | |||
}, | |||
dataset: { | |||
type: Array, | |||
type: [Array, Object], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i know we agreed to not permit this. however, this might be time for an exception or please share a suggestion for how to avoid this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this needs to be an array or more then this needs to be refactored. see line 28
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
proposal: change to "dataAvailable" and make it a boolean. checks are then performed on each page on the individual dataset
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also cheaper to pass
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok makes sense. do you guys think this should be done in this PR and not separately?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's do it in here. the change needs to happen in only 3 places and we don't pile up more dept.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in only 3 places
yeah. one line 😝
:loading="blocks.loading" | ||
:loaded="blocks.loaded" | ||
:error="blocks.error" | ||
:dataset="lastHeader" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
page network is a bit strange because it doesn't need the "items" (dataset) to load. it just needs a connection
to render the top part of the page. the blocks
are the second "dataset". lastHeader
and connection
don't have loading, loaded, or error states...
reveals room for a refactor - but this might be sufficient for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we have in the store the list of blocks, but I like more @faboweb idea of booleanize
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
me too
Codecov Report
@@ Coverage Diff @@
## develop #2214 +/- ##
==========================================
Coverage ? 95.34%
==========================================
Files ? 108
Lines ? 2279
Branches ? 114
==========================================
Hits ? 2173
Misses ? 93
Partials ? 13
|
Codecov Report
@@ Coverage Diff @@
## develop #2214 +/- ##
===========================================
- Coverage 96.63% 96.62% -0.01%
===========================================
Files 109 108 -1
Lines 2316 2314 -2
Branches 116 116
===========================================
- Hits 2238 2236 -2
Misses 65 65
Partials 13 13
|
I think this should be merged right after TmPage and PageNetwork stuff is done, all good and and is a bug fix |
…mos/voyager into jordan/2188-page-network-bug
class="governance" | ||
data-title="Governance" | ||
> | ||
<tm-page :tabs="tabs" class="governance" data-title="Governance"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we cover the case when we have no proposals with the new Boolean dataEmpty?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's true TmPage with tabs hide some logic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's fix the network
Closes #2188
Description:
blocks.loaded
to moduleThank you! 🚀
For contributor:
PENDING.md
with issue # and GitHub usernameFiles changed
in the github PR explorerFor reviewer: