Skip to content

Conversation

@johncox-google
Copy link
Contributor

@marcotuts: I had a bit of spare time so I took a stab at changing the signin controls for social auth from list of text to list of buttons. I know this is something you talked about doing; if you've already done it, let me know and I'll back this out. Otherwise, I'd like to merge it in this week along with https://github.com/edx/edx-platform/pull/2994 and https://github.com/edx/edx-platform/pull/3408.

There's a bit of repeated code because I couldn't figure out how to make vars in a parent mako template visible to a child mako template.

Anyway, this is based off https://github.com/edx/edx-platform/pull/3408 and should not be merged before it. All relevant changes are in 4dd0999.

@nedbat, @jzoldak: if @marcotuts likes the UX, this would resolve one of @jzoldak's concerns at https://github.com/edx/edx-platform/pull/2994#issuecomment-41180064.

@marcotuts
Copy link
Contributor

Thanks @johncox-google for the work done here. I'm spinning this up locally for testing and any potential suggestions, but I did a pure FED code run through and had a couple suggestions. Will note them inline on the last commit. https://github.com/johncox-google/edx-platform/commit/4dd0999eac115cdd1a9276d89a0f4f8acbe0f59d

@marcotuts
Copy link
Contributor

👍 on button styling

@johncox-google
Copy link
Contributor Author

@nedbat Since this PR is a superset of https://github.com/edx/edx-platform/pull/2994 and https://github.com/edx/edx-platform/pull/3408 I did a soft reset off master so we can do one merge rather than three.

While doing final checks I also found a couple corner cases and fixed them in 1e052e3. Could you have a look? If you're happy with the change I'll squash/rebase so you'll be clear to merge.

Thanks, and sorry for the last minute churn.

@johncox-google
Copy link
Contributor Author

@nedbat Cleared up dispatch logic in 1192b5f.

nedbat added a commit that referenced this pull request Apr 25, 2014
Change signin control from text list to buttons.
@nedbat nedbat merged commit 82bd46d into openedx:master Apr 25, 2014
@johncox-google
Copy link
Contributor Author

Forgot to tag @psimakov to keep him in the loop.

@johncox-google johncox-google deleted the johncox/feature/buttons branch April 25, 2014 22:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

open-source-contribution PR author is not from Axim or 2U

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants