Skip to content
This repository has been archived by the owner on Jan 6, 2022. It is now read-only.

Choo v5 #315

Merged
merged 34 commits into from
Mar 27, 2017
Merged

Choo v5 #315

merged 34 commits into from
Mar 27, 2017

Conversation

juliangruber
Copy link
Collaborator

@juliangruber juliangruber commented Mar 22, 2017

A bunch happened here! I hope this isn't too much a pain to review:

  • I changed choo 4 logic to choo 5 logic
  • I refactored the dat manager logic out of the repos model. The dat manager shouldn't be coupled to choo in any way, so it helps in multiple respects to have it live in its own file. While doing that I also simplified it a little
  • I renamed the main-view model to welcome, as it's only responsible for the welcome screen and thus the name is more obvious
  • I renamed the window model to drag-drop, for the same reasons
  • I removed lib/param-router as it wasn't used any more
  • I renamed a bunch of events, to make them more human friendly. Mostly removing namespaces like error:quit -> quit and making them more human like repos:delete -> delete dat. I would see reasons to undo this though as well.
  • I removed cases where a model was using the event system to communicate with other parts of the same model, using functions instead
  • While I was refactoring already, I moved the monkey patching required to run our integration tests to its own lib, to declutter the repos model
  • I refactored pages/main by splitting out elements into elements/empty and elements/welcome. There's now no more css in this file :)
  • In the end, all the QA passed, except known issues found and fixed in master after this branch was created

In total this is -90 lines of code, and a huge speed up (thanks to choo 5)! :)

Once the reviews are done,

  • I'll do the non trivial work of rebasing this branch onto master,
  • and update to use all choo-* dependencies from npm.

@juliangruber
Copy link
Collaborator Author

btw, travis fails because test:integration is still active in this branch, which will be solved after rebasing onto master.

</main>
`
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I think standard might complain about extra newlines

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

do you mean in the future? currently it doesn't

Copy link
Contributor

Choose a reason for hiding this comment

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

oh really? oops might be wrong then 😅 - nevermind ✨

show: false
}, state.welcome)

bus.on('repos loaded', function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

ohhh, we should probably decide on how to name events; in choo we usually delimit using : and camelcase for words. In the dat API we use whitespace to space words, and don't think we have namespaces. I feel we should like agree on something and then stick to it. Perhaps : to namespace and whitespace to split words?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is not a blocker btw, perhaps something to discuss either over text or face? feel we can figure it out in 5mins when talking haha

Copy link
Collaborator

Choose a reason for hiding this comment

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

+1 on having a convention

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok, created an issue here: #318

package.json Outdated
"choo-persist": "^2.0.0",
"choo": "github:yoshuawuyts/choo#v5",
"choo-log": "github:yoshuawuyts/choo-log#v5",
"choo-persist": "github:yoshuawuyts/choo-persist#v5",
Copy link
Contributor

Choose a reason for hiding this comment

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

all versions are published now; install latest should work ✨

@@ -0,0 +1,8 @@
if (process.env.RUNNING_IN_SPECTRON) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not completely sure what this file does; could you maybs add a tiny comment for future reference? Thanks 🙏

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is for mocking the dialog

Copy link
Contributor

@yoshuawuyts yoshuawuyts left a comment

Choose a reason for hiding this comment

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

Neato! - few minor comments but looking good. Think we should def test it a lil more, but so far so good ✨ - digging it just removed 100 lines haha

Copy link
Contributor

@yoshuawuyts yoshuawuyts left a comment

Choose a reason for hiding this comment

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

yup, looking neat!

@juliangruber juliangruber merged commit d91e0f4 into master Mar 27, 2017
@juliangruber juliangruber deleted the choo-v5 branch March 27, 2017 08:49
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants