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

Show connect buttons for installed apps only #3394

Merged
merged 7 commits into from Dec 14, 2017

Conversation

@humitos
Copy link
Member

@humitos humitos commented Dec 11, 2017

Instead of hardcode the button/form for each of the services, we share
the provider_list.html file used in the "Connected Services" tab
from "Account Settings".

This template shows only buttons for the Services installed in the
database.

Pay attention to the CSS changes since I probable screwed something :)

Instead of hardcode the button/form for each of the services, we share
the ``provider_list.html`` file used in the "Connected Services" tab
from "Account Settings".

This template shows only buttons for the Services installed in the
database.
@humitos humitos requested a review from agjohnson Dec 11, 2017
@agjohnson
Copy link
Contributor

@agjohnson agjohnson commented Dec 11, 2017

Could you post some screenshots of the two changes?

@humitos
Copy link
Member Author

@humitos humitos commented Dec 11, 2017

Import project

captura de pantalla_2017-12-11_15-26-07

Settings page

captura de pantalla_2017-12-11_15-26-20

a.socialaccount-provider.github:before {
font-family: FontAwesome;
content: "\f09b";
}

div.project-import-remote form.import-connect-gitlab button:before,
a.socialaccount-provider.gitlab:before {
font-family: FontAwesome;
content: "\f1d3";
Copy link
Member

@safwanrahman safwanrahman Dec 11, 2017

\f296 should be for gitlab
http://fontawesome.io/icon/gitlab/

Copy link
Member Author

@humitos humitos Dec 11, 2017

Nice!

@@ -43,7 +43,7 @@ def get_version_compare_data(project, base_version=None):
}
if highest_version_obj:
ret_val['url'] = highest_version_obj.get_absolute_url()
ret_val['slug'] = highest_version_obj.slug,
ret_val['slug'] = highest_version_obj.slug
Copy link
Member

@ericholscher ericholscher Dec 11, 2017

This looks like an unrelated bug?

Copy link
Member Author

@humitos humitos Dec 11, 2017

Ha! Yeah. I don't understand how this passed the test in the PR that was merged :/

</form>
<div class="project-import-providers">
<ul class="socialaccount_providers">
{% include "socialaccount/snippets/provider_list.html" with process="connect" %}
Copy link
Member

@ericholscher ericholscher Dec 11, 2017

I don't see this file in the commit, did it already exist?

Copy link
Member

@ericholscher ericholscher Dec 11, 2017

Oh I see it below -- is it also used somewhere else?

Copy link
Member Author

@humitos humitos Dec 11, 2017

Yes, it is used in the /accounts/social/connections/ page, under the admin settings.

Copy link
Contributor

@agjohnson agjohnson Dec 11, 2017

We do lose the next parameter here, correct? This will likely just redirect users back to the connection page. Is redirecting to the import page important here?

Copy link
Member Author

@humitos humitos Dec 12, 2017

Good catch! I didn't realize this.

Yes, I think it's important since you are at the import page. I'd like to connect my account and import the my project immediately. I'm proposing a fix for this using the template tag: https://github.com/pennersr/django-allauth/blob/cfc5112bf474af2cbb16e4b4a36708e8949a2105/allauth/socialaccount/templatetags/socialaccount.py#L45

Copy link
Contributor

@agjohnson agjohnson left a comment

I like the approach here, it's a good chance to reuse parts of allauth. If you need anything on styling, feel free to assign me.

a.socialaccount-provider.gitlab:before {
font-family: FontAwesome;
content: "\f1d3";
}

div.project-import-remote form.import-connect-bitbucket button:before,
Copy link
Contributor

@agjohnson agjohnson Dec 11, 2017

What's the reason we're removing this styles?

Copy link
Member Author

@humitos humitos Dec 12, 2017

I removed these because the form is not used anymore (I removed them). So, I didn't want to leave orphans styles...

</form>
<div class="project-import-providers">
<ul class="socialaccount_providers">
{% include "socialaccount/snippets/provider_list.html" with process="connect" %}
Copy link
Contributor

@agjohnson agjohnson Dec 11, 2017

We do lose the next parameter here, correct? This will likely just redirect users back to the connection page. Is redirecting to the import page important here?

div.project-import-remote form.import-connect button {
display: inline;
div.project-import-remote a.socialaccount-provider {
display: block;
Copy link
Contributor

@agjohnson agjohnson Dec 11, 2017

These changes look to have some negative side effect on spacing and alignment. What prompted these changes? If we have to alter layout on one of the forms, that might be better.

Copy link
Member Author

@humitos humitos Dec 12, 2017

"I don't know CSS", so I don't have good reasons. I touched it a little, googled something and it looked it worked so I thought that I could propose it.

I'm totally 👍 on remove this and change it as you like/prefer/wish/etc.

Copy link
Contributor

@agjohnson agjohnson Dec 12, 2017

Also, missed this the first time. This is actually editing a compiled css file, the source is at readthedocs/projects/static-src/projects/css/import.less. I'm going to be working to unify our styling, and to get rid of our core.css file soon.

@agjohnson
Copy link
Contributor

@agjohnson agjohnson commented Dec 12, 2017

Updated the styles for the import list page:

image

@agjohnson agjohnson merged commit 00b081d into master Dec 14, 2017
2 checks passed
@agjohnson agjohnson deleted the humitos/services/show-only-enable branch Dec 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants