replace microcomponent with nanocomponent#450
replace microcomponent with nanocomponent#450aks- wants to merge 18 commits intodat-ecosystem-archive:masterfrom
Conversation
|
I see that the appveyor build was already failing, so I didn't go fix that |
juliangruber
left a comment
juliangruber
left a comment
There was a problem hiding this comment.
...otherwise, this looks great to me! thank you so much for putting in all the work :)
| @@ -1,6 +1,6 @@ | |||
| 'use strict' | |||
There was a problem hiding this comment.
somehow this broke the hamburger menu, it's not opening up any more for me
There was a problem hiding this comment.
Hm, I will take a look when I have time
There was a problem hiding this comment.
fixed by @martinheidegger in this commit 04697c0 yay!
|
I had to tweet about this: https://twitter.com/juliangruber/status/952194559328518149 (couldn't find your twitter handle) |
|
@juliangruber thanks for the fantastic praise, I have also forgotten twitter handle, haven't used it in long time. Thank you again :) |
aks-
force-pushed
the
refactor-with-nanocomponent
branch
from
04697c0 to
1b01530
Compare
|
Still requires some fix, I will do it when I have time and let you folks know. Currently overloaded at work... |
…e some problems in debugging and seems like a violation of control flow; so I extracted the logic into a functional method.
…rings don’t occur so frequently by using .render instead of .createElement
aks-
force-pushed
the
refactor-with-nanocomponent
branch
from
1b01530 to
56229d3
Compare
|
@juliangruber updated PR, can you please check it? |
|
side note: please try in the future not to rebase such huge PRs, it can be hard to tell what you have reviewed already and what not |
elements/header.js
Outdated
| <header class="${header}"> | ||
| <div class="fr relative"> | ||
| ${importButton.render({ | ||
| ${importButton.createElement({ |
There was a problem hiding this comment.
this should still use importButton.render, createElement is a private api. See https://github.com/choojs/nanocomponent#usage
elements/table-row.js
Outdated
| if (!(this instanceof Row)) return new Row() | ||
| Nanocomponent.call(this) | ||
|
|
||
| this.parts = { |
There was a problem hiding this comment.
parts is a bit generic, can we call this partials maybe? That term is known from the Ruby on Rails world
| } | ||
|
|
||
| module.exports = { | ||
| datPeers, |
There was a problem hiding this comment.
why not inline datPeers here too?
There was a problem hiding this comment.
datPeers is used in datState. If datState would use this.datPeers to access it the method could not be able to simply use it like: var {datState} = require('./lib/datInfo')
|
I hope this is the last big view refactor. Thank you so much for going through all this work! |
|
@juliangruber can we remove appveyor tests for windows? or maybe try fixing them by updating deps for hypercore, hyperdive? |
|
Afaik hypercore/hyperdrive isn't breaking the tests on windows, there were UI issues instead |
|
@juliangruber hm I see... I haven't got windows machine otherwise I could have tried fixing it... btw would you want anything else changed in this PR? |
|
Since windows was failing before, I'm fine with merging this as long as all on the mac / travis side still works. I'll give this a last read through and then will merge! |
|
Does downloading dats work for you? I get this screen after I tried to download dat://40a7f6b6147ae695bcbcff432f684c7bb5291ea339c28c1755896cdeb80bd2f9/:
|
|
works, downloaded successfully. |
|
Will the work on this continue or should it be closed in favor of #457 ? |
|
React branch used, closing this. |
4 participants
I refactored this, replacing microcomponent with nanocomponent, as in #446, I will do the others in another PR