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.

Sign up for GitHub

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

Jump to bottom

Factor out providers from accounts ui unstyled #572

Closed
wants to merge 12 commits into from
Closed

Factor out providers from accounts ui unstyled #572

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
865776a
Removed linkedin package from this PR
zealoushacker Dec 22, 2012
1034514
This was never supposed to be a part of this PR either - removed
zealoushacker Dec 22, 2012
83627e4
Add twitter configure button logo css
zealoushacker Dec 24, 2012
2d8402f
Tests for the functionality we want
zealoushacker Dec 26, 2012
b11204f
Accounts._loginButtons.getLoginServices returns unique services
zealoushacker Dec 26, 2012
326af44
Add password to the available service buttons if it's included
zealoushacker Dec 26, 2012
c27c482
Vastly simplify by removing the JSON parsing.
zealoushacker Dec 26, 2012
63e172e
Variable declarations on one line; self instead of that.
zealoushacker Dec 26, 2012
3746d38
Remove uniqueness check. Devs shouldn't be idiots :)
zealoushacker Dec 26, 2012
504e565
Make sure password is last
zealoushacker Dec 26, 2012
c50a06f
Oops, we should memoize the last service in case we need to swap
zealoushacker Dec 27, 2012
4c51764
Remove whitespace
zealoushacker Dec 27, 2012
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions packages/accounts-facebook/accounts_facebook.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
#login-buttons-image-facebook {
background-image: url(data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAABAAAAAQCAYAAAAf8/9hAAAACXBIWXMAAAsTAAALEwEAmpwYAAAAq0lEQVQ4jWP8//8/AyWAhYGBgcEmauYZBgYGYxL1nj2yLN2ECcohVTNcDwsxKlXlhRm6yzwZRAS5GRgYGBhsombC5ZhwaUIGyJrRAVEuwGYzSS7AB/C64MiydKx8ZJfgNeDN+68MDAwIL8D4RLsgIHsJis0wPjKgOAyoE4hcnGwMGkpiBBUbacvA2TfuvaKiC759/3X23NUnOPMDtgTEwMBwloGBgYGR0uwMAGOPLJS9mkQHAAAAAElFTkSuQmCC);
}
1 change: 1 addition & 0 deletions packages/accounts-facebook/facebook_client.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
Accounts.oauth.initiateLogin(state, loginUrl, callback);
};

Accounts._loginButtons.loginServices.push('facebook');
Copy link
Contributor

Choose a reason for hiding this comment

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

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...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

})();


Expand Down
2 changes: 1 addition & 1 deletion packages/accounts-facebook/package.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ Package.on_use(function(api) {
api.use('templating', 'client');

api.add_files(
['facebook_configure.html', 'facebook_configure.js'],
['accounts_facebook.css', 'facebook_configure.html', 'facebook_configure.js'],
'client');

api.add_files('facebook_common.js', ['client', 'server']);
Expand Down
3 changes: 3 additions & 0 deletions packages/accounts-github/accounts_github.css
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions packages/accounts-github/github_client.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,4 +25,6 @@

Accounts.oauth.initiateLogin(state, loginUrl, callback, {width: 900, height: 450});
};

Accounts._loginButtons.loginServices.push('github');
}) ();
2 changes: 1 addition & 1 deletion packages/accounts-github/package.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ Package.on_use(function(api) {
api.use('templating', 'client');

api.add_files(
['github_configure.html', 'github_configure.js'],
['accounts_github.css', 'github_configure.html', 'github_configure.js'],
'client');

api.add_files('github_common.js', ['client', 'server']);
Expand Down
3 changes: 3 additions & 0 deletions packages/accounts-google/accounts_google.css
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions packages/accounts-google/google_client.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,4 +36,5 @@
Accounts.oauth.initiateLogin(state, loginUrl, callback);
};

Accounts._loginButtons.loginServices.push('google');
}) ();
2 changes: 1 addition & 1 deletion packages/accounts-google/package.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ Package.on_use(function(api) {
api.use('templating', 'client');

api.add_files(
['google_configure.html', 'google_configure.js'],
['accounts_google.css', 'google_configure.html', 'google_configure.js'],
'client');

api.add_files('google_common.js', ['client', 'server']);
Expand Down
3 changes: 2 additions & 1 deletion packages/accounts-password/password_client.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,5 +155,6 @@
methodArguments: [token],
userCallback: callback});
};
})();

Accounts._loginButtons.loginServices.push('password');
})();
3 changes: 3 additions & 0 deletions packages/accounts-twitter/accounts_twitter.css
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion packages/accounts-twitter/package.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ Package.on_use(function(api) {
api.use('templating', 'client');

api.add_files(
['twitter_configure.html', 'twitter_configure.js'],
['accounts_twitter.css', 'twitter_configure.html', 'twitter_configure.js'],
'client');

api.add_files('twitter_common.js', ['client', 'server']);
Expand Down
1 change: 1 addition & 0 deletions packages/accounts-twitter/twitter_client.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,4 +31,5 @@
Accounts.oauth.initiateLogin(state, url, callback);
};

Accounts._loginButtons.loginServices.push('twitter');
})();
1 change: 0 additions & 1 deletion packages/accounts-ui-unstyled/accounts_ui_tests.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,4 +14,3 @@ Tinytest.add('accounts-ui - config validates keys', function (test) {
Accounts.ui.config({requestPermissions: {facebook: "not an array"}});
});
});

22 changes: 22 additions & 0 deletions packages/accounts-ui-unstyled/accounts_ui_unstyled_tests.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
(function(environment) {
// setup
Accounts._loginButtons.loginServices.push('password');
environment.meteorServices = function() {
return Accounts._loginButtons.getLoginServices();
};

Tinytest.add(
'accounts-ui - getLoginServices retuns an array of service hashes',
function (test) {
test.equal(_.first(environment.meteorServices()), {name: "password"});
}
);

Tinytest.add(
'accounts-ui - getLoginServices should always return password last',
function (test) {
Accounts._loginButtons.loginServices.push('some_other_service');
test.equal(_.last(environment.meteorServices()), {name: "password"});
}
);
})(Tinytest);
25 changes: 16 additions & 9 deletions packages/accounts-ui-unstyled/login_buttons.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,18 +115,25 @@
return '';
};

Accounts._loginButtons.loginServices = [];

Accounts._loginButtons.getLoginServices = function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

var ret = [];
var self = this,
services = self.loginServices, // memoize services array
passwordIndex = services.indexOf("password"), // memoize password idx.
lastServiceAt = services.length - 1, // memoize last service idx.
lastService = _.last(services); // in case we need to swap anything

// make sure to put password last, since this is how it is styled
// in the ui as well.
_.each(
['facebook', 'github', 'google', 'twitter', 'weibo', 'password'],
function (service) {
if (Accounts[service])
ret.push({name: service});
});
// if we had found password, swap w last service
if (passwordIndex !== -1) {
services[lastServiceAt] = services[passwordIndex];
services[passwordIndex] = lastService;
}

return ret;
return _.map(services, function(service) {
return {name: service};
});
};

Accounts._loginButtons.hasPasswordService = function () {
Expand Down
21 changes: 0 additions & 21 deletions packages/accounts-ui-unstyled/login_buttons_images.css
View file
Open in desktop

This file was deleted.

2 changes: 1 addition & 1 deletion packages/accounts-ui-unstyled/package.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ Package.on_use(function (api) {
api.add_files([
'accounts_ui.js',

'login_buttons_images.css',
'login_buttons.html',
'login_buttons_single.html',
'login_buttons_dropdown.html',
Expand All @@ -26,4 +25,5 @@ Package.on_test(function (api) {
api.use('accounts-ui-unstyled');
api.use('tinytest');
api.add_files('accounts_ui_tests.js', 'client');
api.add_files('accounts_ui_unstyled_tests.js', 'client');
});
3 changes: 3 additions & 0 deletions packages/accounts-weibo/accounts_weibo.css
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion packages/accounts-weibo/package.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ Package.on_use(function(api) {
api.use('templating', 'client');

api.add_files(
['weibo_configure.html', 'weibo_configure.js'],
['accounts_weibo.css', 'weibo_configure.html', 'weibo_configure.js'],
'client');

api.add_files('weibo_common.js', ['client', 'server']);
Expand Down
1 change: 1 addition & 0 deletions packages/accounts-weibo/weibo_client.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,4 +25,5 @@
Accounts.oauth.initiateLogin(state, loginUrl, callback);
};

Accounts._loginButtons.loginServices.push('weibo');
}) ();