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

Refactoring, short-circuiting #4

Merged
merged 13 commits into from
Apr 26, 2016
Merged

Refactoring, short-circuiting #4

merged 13 commits into from
Apr 26, 2016

Conversation

lorensr
Copy link
Collaborator

@lorensr lorensr commented Apr 25, 2016

Not ready to merge, I haven't tested yet. Probably even has syntax errors 😜

Changes are more intelligible if you look at individual commits.

  • In general when I've seen calling something not production-ready, it's sort of a vague unsubstantiated insult. Like the big firm that told me Meteor 0.9 wasn't production ready. Of course it was. So I think it's better to instead say why you're better, and that's more options and launch screen UX.
  • tells them what they can do with pkg before getting into api
  • ToC
  • get them to use Reloader.reload() so we can keep reloaderWasRefreshed accurate.
  • launchScreenDelay
  • moved functions into Reloader for semantics and ability to use this, and now it's easier to see what's going on at the top level at the bottom of the file
  • minor DRYing
  • i'd recommend being more sparing with comments, in favor of writing readable code 😊 for example, i believe this:
// Hold the launch screen
const handle = LaunchScreen.hold();

// Just release the splash screen
handle.release();

takes longer to read and understand than this:

const launchScreen = LaunchScreen.hold();

launchScreen.release();

a lot of readbility has to do with naming and structure (handle -> launchScreen). similarly, note how I extracted out pieces of code, or compound booleans, into functions with names that were descriptive enough that no comment was necessary.

@lorensr
Copy link
Collaborator Author

lorensr commented Apr 25, 2016

Did your test app have much in it, or just a call to configure that you manually changed during testing?

@jamielob
Copy link
Owner

This looks great @lorensr! I'll take a better look tomorrow. Test app was just a manual testing app.

@lorensr
Copy link
Collaborator Author

lorensr commented Apr 26, 2016

tests don't work, waiting on meteor/guide#401

@lorensr
Copy link
Collaborator Author

lorensr commented Apr 26, 2016

@lorensr
Copy link
Collaborator Author

lorensr commented Apr 26, 2016

tests working, would you like to add to them?

@lorensr
Copy link
Collaborator Author

lorensr commented Apr 26, 2016

production-ready packages have tests and changelogs 😉 😄 good thing we're past 1.3 and don't have to use tinytest! https://github.com/lorensr/login-links/blob/master/tests/client/connectionLogin.js

@jamielob
Copy link
Owner

You're a star! I'm just finishing up some contract work and merging all this is next on my list.

@jamielob jamielob merged commit 11248d6 into jamielob:master Apr 26, 2016
@lorensr
Copy link
Collaborator Author

lorensr commented Apr 26, 2016

Thanks! It was enough changes that I wouldn't publish until more tests are written or it's re-manually tested

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.

2 participants