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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Factor out providers from accounts ui unstyled #572

Closed
wants to merge 12 commits into
base: devel
from

Conversation

Projects
None yet
2 participants
@zealoushacker
Contributor

zealoushacker commented Dec 26, 2012

@avital, after thinking about your comments in #566 some more, I realized that I was also missing a test. Upon writing the test, I had also noticed that we were not accounting for dupe providers pushed onto Accounts._loginButtons.loginServices. I also realized that I had forgotten to add the password package's initialization statement, in the case that it's included ^_^. Finally, I removed the if clause that you had identified correctly as superfluous.

Thanks for the feedback.

FYI, would help to add this to the official docs:
http://stackoverflow.com/questions/10760601/how-do-you-run-the-meteor-tests/14035031#14035031

Oh, and any plans to put this on Travis? That would be 馃殌!

zealoushacker added some commits Dec 22, 2012

Removed linkedin package from this PR
Login services should be defined from within their packages

Corresponding login service assets (images and css should be in their respective packages not inside of accounts-ui-unstyled)

Conflicts:
	packages/accounts-linkedin/linkedin_client.js
	packages/accounts-linkedin/package.js
	packages/accounts-ui-unstyled/login_buttons.js
	packages/accounts-ui-unstyled/login_buttons_images.css
Add twitter configure button logo css
Oops, had failed to move that over
Accounts._loginButtons.getLoginServices returns unique services
This may be improved by passing an iterator to _.uniq instead of doing
this malarkey I did for the first run with JSON stringify and parse. ^_^
Vastly simplify by removing the JSON parsing.
Instead, just run _.uniq on the Accounts._loginButtons.loginServices.
DUH. PRESTO!
return ret;
Accounts._loginButtons.getLoginServices = function () {
var that = this, services = that.loginServices;
return _.map(_.uniq(services), function(service) {

This comment has been minimized.

@avital

avital Dec 26, 2012

Contributor

I'm curious -- in what case could you end up with duplicate services?

This comment has been minimized.

@zealoushacker

zealoushacker Dec 26, 2012

Contributor

If someone pushes a dupe onto the very public Accounts._loginButtons.loginServices.

This comment has been minimized.

@avital

avital Dec 26, 2012

Contributor

Do you mean if an app developer intentionally changes the value of Accounts._loginButtons.loginServices? I don't think that's a real concern. Similarly, someone can also do something like: Accounts._loginButtons.loginServices = "WON'T WORK", if they choose to shoot themselves in the foot.

return ret;
Accounts._loginButtons.getLoginServices = function () {
var that = this, services = that.loginServices;

This comment has been minimized.

@avital

avital Dec 26, 2012

Contributor

We use self rather than that in these cases.

Also, please don't define more than one var on each line.

@@ -115,18 +115,13 @@
return '';
};
Accounts._loginButtons.getLoginServices = function () {
var ret = [];
// make sure to put password last, since this is how it is styled

This comment has been minimized.

@avital

avital Dec 26, 2012

Contributor

Does accounts-ui still work now that we don't guarantee that password is last? As far as I can tell, you removed this comment and related functionality without replacing it with anything equivalent.

Alternatively, we could refactor accounts-ui to not require password to be last in the list but just to render them completely separately.

BTW, a helpful way to test changes to accounts-ui is by using the accounts-ui-viewer app in examples/unfinished/accounts-ui-viewer.

@avital

This comment has been minimized.

Contributor

avital commented Dec 26, 2012

As for using travis, we will have to definitely invest in our testing infrastructure going forward, and @sixolet seems to be taking some initiative on that... But it'll take a while.

Also, I've recently learned about circleci, which claims to be like travis but better (and faster)... but we'd have to look into each to evaluate them...

@zealoushacker

This comment has been minimized.

Contributor

zealoushacker commented Dec 26, 2012

馃憤

Looking forward to more work on testing infrastructure. Happy to help too. Will make your release cycles tighter. Probably preaching to the choir.

I use circleci for my private projects! :) It's very awesome, but I don't think they are free for open source, whereas Travis is.

@avital

This comment has been minimized.

Contributor

avital commented Dec 27, 2012

Thanks! This looks good, and I'm happy you wrote tests. I'm going to make some changes on top of this and then merge them all of the changes onto devel.

@@ -28,6 +28,7 @@
Accounts.oauth.initiateLogin(state, loginUrl, callback);
};
Accounts._loginButtons.loginServices.push('facebook');

This comment has been minimized.

@avital

avital Dec 27, 2012

Contributor

This code doesn't work in case the facebook package was added before accounts-ui. Don't worry about fixing this -- I'm redo'ing the login service listing to iterate over the Meteor.loginWith{ServiceName} methods instead, which I believe won't have the problem because all javascript must have been loaded before any template helpers are called.

I think the next step (outside this PR) would be to eliminate all of the boilerplate code in files like facebook_client.js. It could be pretty easy to create a function in accounts-base/accounts_client.js named, say, Accounts.registerLoginService that does this...

This comment has been minimized.

@zealoushacker

zealoushacker Dec 27, 2012

Contributor

This code doesn't work in case the facebook package was added before accounts-ui.

Ah, indeed. Dependencies...

I'm redo'ing the login service listing to iterate over the Meteor.loginWith{ServiceName} methods instead

Do you have a branch where you're doing that work? I'd love to take a look and/or contribute.

I think the next step (outside this PR) would be to eliminate all of the boilerplate code in files like facebook_client.js. It could be pretty easy to create a function in accounts-base/accounts_client.js named, say, Accounts.registerLoginService that does this...

Was definitely my plan - I think Accounts.registerLoginService would be the correct sort of abstraction that I had in mind as well for later :) Wanted to get something out there first, and then iterate.

This comment has been minimized.

@avital

avital Dec 27, 2012

Contributor

Just pushed my work-in-progress onto branch avital-pr-572. I'm going to rename the accounts_github.css, etc files and then I think I'm done.

@zealoushacker

This comment has been minimized.

Contributor

zealoushacker commented Dec 27, 2012

I'm going to make some changes on top of this and then merge them all of the changes onto devel.

馃憤 Looking forward to it.

@avital

This comment has been minimized.

Contributor

avital commented Dec 27, 2012

Merged into devel, with some rebasing and two changes on top: a0b497e and de90c75.

There is an XXX comment describing how we can make it easier to write additional login services. If you are interesting in digging into this that's be really helpful.

@avital

This comment has been minimized.

Contributor

avital commented Dec 27, 2012

Thanks again for all of this work, and the many comments back and forth!

@avital avital closed this Dec 27, 2012

@zealoushacker

This comment has been minimized.

Contributor

zealoushacker commented Dec 27, 2012

馃憤 Reviewing your changes to learn a thing or two :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment