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

Projects
None yet
4 participants
@humitos
Member

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 :)

Show connect buttons for installed apps only
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

This comment has been minimized.

Contributor

agjohnson commented Dec 11, 2017

Could you post some screenshots of the two changes?

humitos added some commits Dec 11, 2017

@humitos

This comment has been minimized.

Member

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";

This comment has been minimized.

@safwanrahman

This comment has been minimized.

@humitos

humitos Dec 11, 2017

Member

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

This comment has been minimized.

@ericholscher

ericholscher Dec 11, 2017

Member

This looks like an unrelated bug?

This comment has been minimized.

@humitos

humitos Dec 11, 2017

Member

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" %}

This comment has been minimized.

@ericholscher

ericholscher Dec 11, 2017

Member

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

This comment has been minimized.

@ericholscher

ericholscher Dec 11, 2017

Member

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

This comment has been minimized.

@humitos

humitos Dec 11, 2017

Member

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

This comment has been minimized.

@agjohnson

agjohnson Dec 11, 2017

Contributor

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?

This comment has been minimized.

@humitos

humitos Dec 12, 2017

Member

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

@agjohnson

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,

This comment has been minimized.

@agjohnson

agjohnson Dec 11, 2017

Contributor

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

This comment has been minimized.

@humitos

humitos Dec 12, 2017

Member

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" %}

This comment has been minimized.

@agjohnson

agjohnson Dec 11, 2017

Contributor

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;

This comment has been minimized.

@agjohnson

agjohnson Dec 11, 2017

Contributor

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.

This comment has been minimized.

@humitos

humitos Dec 12, 2017

Member

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

This comment has been minimized.

@agjohnson

agjohnson Dec 12, 2017

Contributor

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.

humitos and others added some commits Dec 12, 2017

@agjohnson

This comment has been minimized.

Contributor

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

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@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