Add Services and Privacy settings page to mobile #6086
Conversation
|
|
||
| .container-fluid |
There was a problem hiding this comment.
Git diff is not really powerful here, the only thing I did was to remove a .container-fluid and so moving everything two spaces to the left.
There was a problem hiding this comment.
This adds some inconsistency for the desktop views. /user/edit has no surrounding .container-fluid anymore while all other settings pages still have one.
|
Wow, no hound remark at all? Wasn't expected. |
| @@ -2,13 +2,14 @@ | |||
| -# licensed under the Affero General Public License version 3 or later. See | |||
| -# the COPYRIGHT file. | |||
|
|
|||
| = render "shared/settings_nav" | |||
| .stream_element | |||
There was a problem hiding this comment.
That class doesn't look correct in this context. Maybe think of a new name and apply the same styles? (Or pick those that are necessary in this case)
There was a problem hiding this comment.
Yeah I agree. I think that we should have two layouts in the mobile version, the pages which contain elements, like the stream, the profile, the notifications or the contacts pages, and the pages are more about text, like the settings pages, the admin panel, the getting started...
The pages of the first group should have the grey background and then a white bg around each elements, but the text pages should directly have a white background, to avoid losing width space and look overloaded.
What do you think about that? It's a bigger work than what I'm doing here, that's why I simply used .stream_element as a temporary solution, but I agree, temp can become long, so I can make this one cleaner.
|
This closes #5312 btw |
| = block.person_name | ||
| \- | ||
| = link_to t('.stop_ignoring'), block_path(block), | ||
| method: :delete |
There was a problem hiding this comment.
No way to render the desktop view instead ?
There was a problem hiding this comment.
I could use a partial to do that yeah. What's your opinion everybody? Is this view good enough like this for mobile or should it be improved?
There was a problem hiding this comment.
I extracted it in a partial.
Flaburgan
force-pushed
the
add-services-mobile
branch
2 times, most recently
from
07ea19d
to
eb2bb78
Compare
|
Rebased, this is ready to review. |
|
I'll review this tonight. (Unless someone else is faster) |
| .clearfix= f.submit t('users.edit.close_account_text'), class: "btn btn-danger pull-right", | ||
| id: "close_account_confirm", data: { confirm: t("are_you_sure_delete_account") } | ||
|
|
||
| - if not mobile |
There was a problem hiding this comment.
Shouldn't this be just - else?
There was a problem hiding this comment.
I didn't touch this code at all, I only reindented it that's why it is shown as modified.
There was a problem hiding this comment.
I removed that in #6102 so don't worry about it anymore.
|
Hm, you removed the users/_edit.haml file in |
|
@Flaburgan Because there were no differences between the rendered files anymore. |
|
Well, in develop, but not in this PR... |
|
If you need different views for desktop and mobile you can move |
|
The partial is fine, I'm going to redo it. I just wanted to check if you did that for a good reason or just as a refactor (it seemed weird because you reviewed this code just before). Anyway, back to work :) |
Flaburgan
force-pushed
the
add-services-mobile
branch
from
1c818d5
to
1b0b059
Compare
|
I refactored the settings pages to correctly use the bootstrap grid. I'll have a final look at it soon and then it'll be ready. |
Flaburgan
force-pushed
the
add-services-mobile
branch
from
1b0b059
to
612ec1c
Compare
|
This should be ready. |
| @@ -0,0 +1,2 @@ | |||
| .container-fluid | |||
| = render "edit", mobile: false | |||
There was a problem hiding this comment.
the variable mobile is not used anywhere
|
services before:
services after:
|
Flaburgan
force-pushed
the
add-services-mobile
branch
from
612ec1c
to
3e4b850
Compare
|
@svbergerem problems fixed. Btw @goobertron is |
|
You are a native English speaker! Yes, 'There are no services available on this pod.' is far better - grammatically correct, and clearer in meaning. I'm just going to have a think about whether there's a more 'positive'/friendly way we can say this. Probably not, and it's fine as is - just a good opportunity to see whether there's any improvement that can be made. |
|
Maybe 'There are no third party sharing services available on this pod.' is clearer? Just plain 'services' is a bit ambiguous in terms of what 'services' are if you didn't know what they could be in the first place. Without context, it might even make it sound like this pod can't do anything. |
|
There is context given by the notice to the right in the Services tab - see screenshots above. However it's not currently that informative. I like your 'third-party sharing services', which I think makes it a bit clearer. I would add this to the notice on the right-hand side, so:
on the right, and stay with
for the notice when there are none connected. Fla, would you be happy to incorporate that extra change to the en locale in this PR? |
|
@goobertron done. |
Add Services and Privacy settings page to mobile
|
Thank you! |
|
Nice :) |
No description provided.