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

[WIP] Hide the art container if images are disabled #43

Closed
wants to merge 4 commits into from
Closed

[WIP] Hide the art container if images are disabled #43

wants to merge 4 commits into from

Conversation

stuxo
Copy link
Contributor

@stuxo stuxo commented Sep 21, 2015

Extremely minor code change, but it does look quite nice (in my opinion). Most of my app usage is on mobile data and browsing the forums is a bit of a pain because of all the wasted space in a post if avatars are disabled.

With this change, if you then enable images, it makes the text view feel very small again. I might experiment with text wrapping around the avatar, or starting the text below the avatar or perhaps moving the avatar entirely.

Let me know what you guys think.

@Twinklebear
Copy link
Collaborator

Cool! Does this handle the case where images may be disabled/enabled while in the view? For example the user is viewing some comments with images disabled, goes into settings and enables images, do we properly re-show the view?

Having the text wrap around the avatar was the solution I was hoping to put together but didn't have time to come back to it. I think having the text below the avatar might leave too much empty space since next to the avatar will all be blank. If there's a better way to layout the comments I'm open to ideas though.

@stuxo
Copy link
Contributor Author

stuxo commented Sep 22, 2015

Since it only checks if (imagesEnabled) when the activity loads, the updated setting will only be visible when loading another activity or reloading the current one. So in the example of the forums, if they were in a thread and changed the setting, they would have to leave the thread and enter it again to see the change.

As for the wrapping, its likely we would have to use something like this. I do hate adding dependancies, but it looks reasonably promising. I'll give it a go anyway, to see how it looks. I'll post results :)

Edit:
forums

This is just a rough implementation. It doesn't seem to render correctly.

Twinklebear added a commit that referenced this pull request Oct 21, 2015
@Twinklebear
Copy link
Collaborator

Manually merged in the remaining changes from this PR to avoid dealing with more merge conflicts 😛 . Thanks @stuxo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants