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

Choo #314

Merged
merged 6 commits into from
Nov 22, 2017
Merged

Choo #314

merged 6 commits into from
Nov 22, 2017

Conversation

alanshaw
Copy link
Member

@alanshaw alanshaw commented Nov 17, 2017

I'm just going to park this here...

The UI interactions are currently old school manual DOM mutations. The complexity of the browser actions popup has got to a point where managing it is getting tricky. I thought it might help to introduce a framework to alleviate this problem so this PR adds choo, "A 4kb framework for creating sturdy frontend applications" 🚂 🚋 🚋 😛

Choo is kinda like React in that the UI is driven by state and is kinda like redux in that a route has a "store" where state is initialised and mutated. Components (just functions) deal with what should be shown/hidden based on the state that is provided to them. So instead of manipulating individual DOM elements when things happen, we simply set some state and let the components work out what they should look like.

I'm totally open for this not being accepted or switched up for React or something. I thought that choo would have the smallest impact on bundle size and also means we don't have to do any transpiling (like we would have to do for jsx) so we're still using vanilla JS - hooray.

N.B. if you're twitchy about the lack of HTML syntax highlighting and are using Atom then do this
N.B. this branch is based on the current browserify branch - I'll rebase when/if it gets merged 😄
N.B. it's worth looking at just the choo changes
N.B. tests need to get fixed

Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

Wow, thank you for taking care of this technical debt! 💰 ✨

I'm totally open for this not being accepted or switched up for React or something. I thought that choo would have the smallest impact on bundle size and also means we don't have to do any transpiling (like we would have to do for jsx) so we're still using vanilla JS - hooray.

Choo is a good call, it is more than enough for current UI.
It solves multiple problems without ballooning our dependencies. 👌

I think it will be OK merge this after #311, but before it happens we need to address a small regression:
There is a delay before browser action popup is rendered for current IPFS resource (we see "offline" state for a fraction of a second):

peek 2017-11-18 19-53

I did not look into it, just asking: is it something we can easily fix on our side, or is it cost of using Choo?

@alanshaw
Copy link
Member Author

@lidel thanks for the kind comments!

is it something we can easily fix on our side, or is it cost of using Choo?

It's not a limitation of Choo or caused by it, I actually added the delay as I noticed the popup wasn't rendering correctly when it received an update as it was animating in. This was prior to adding choo and is probably out of scope for this PR so I'll remove it.

@lidel
Copy link
Member

lidel commented Nov 20, 2017

@alanshaw Bam! I merged #311 -- feel free to rebase this PR 👌

@alanshaw
Copy link
Member Author

@lidel I've rebased, but I'm getting this error when adding the extension to chrome:

screen shot 2017-11-21 at 09 07 32

@lidel
Copy link
Member

lidel commented Nov 21, 2017

@alanshaw that's my fault, I've committed Firefox-specific version suffix in manifest.json, my apologies.

Please remove the "rc2" suffix, it will fix the error under Chrome.

@lidel lidel merged commit 5af3e26 into ipfs:master Nov 22, 2017
@lidel lidel added the kind/maintenance Work required to avoid breaking changes or harm to project's status quo label Nov 22, 2017
@lidel
Copy link
Member

lidel commented Nov 22, 2017

Choo is now in master. Thank you @alanshaw !

I made a test release with it: 🚂🚋🚋🚋v2.1.0rc2🚋🚋

@alanshaw alanshaw deleted the choo branch November 22, 2017 09:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/maintenance Work required to avoid breaking changes or harm to project's status quo
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants