Skip to content

Material Design Overhaul / Day Night Theme [Ready For Review]#47

Merged
Twinklebear merged 23 commits intoGwindow:developfrom
stuxo:Updated-Profile
Mar 29, 2016
Merged

Material Design Overhaul / Day Night Theme [Ready For Review]#47
Twinklebear merged 23 commits intoGwindow:developfrom
stuxo:Updated-Profile

Conversation

@stuxo
Copy link
Copy Markdown
Contributor

@stuxo stuxo commented Mar 6, 2016

No description provided.

@stuxo stuxo changed the title Material Design Overhaul [WIP] Material Design Overhaul / Day Night Theme [Ready For Review] Mar 13, 2016
@Twinklebear
Copy link
Copy Markdown
Collaborator

The screenshots you posted on the forums of the light/dark themes look really nice! Let me know when this is closer to being ready and I'll do some testing as well.

Edit: I just saw you changed the title, I'll check it out :)

@stuxo
Copy link
Copy Markdown
Contributor Author

stuxo commented Mar 14, 2016

I'm quite happy with it now @Twinklebear, I dropped the [WIP] from the pull request. Its feature complete, ideally the only changes I make will be based off your's and other's feedback.

Edit: If you have any suggestions as to light theme colour schemes (or even if you like the teal) please let me know, I'm a bit unsure of it.

@Twinklebear
Copy link
Copy Markdown
Collaborator

It looks like the Tags & Similar Artists headers for the torrent group description wasn't re-styled, same with various headers for a request (the artists & top contributors are updated but it seems like tags/acceptable media etc. aren't)

It also seemed to start me in the light material theme even though the setting wasn't selected. The login progress dialog also seems to flash as white when using the dark theme.

Besides those smaller things this looks really good, I think the teal is pretty nice.

@stuxo
Copy link
Copy Markdown
Contributor Author

stuxo commented Mar 15, 2016

Updated @Twinklebear, please check out the changes on the Request page and the profile page (recent snatches and uploads with images enables/disabled)

@Twinklebear
Copy link
Copy Markdown
Collaborator

It looks like I still get defaulted to the light theme although the setting isn't checked, where the artist page tags/similar artists headers updated as well?

It also seems like the card views shown for uploads/snatches may not be getting updated when changing to the dark theme (they did switch previously iirc). I've been testing on Android 6

@stuxo
Copy link
Copy Markdown
Contributor Author

stuxo commented Mar 15, 2016

I've been testing on android 6 too, I can't reproduce the issue where you start in light theme. It could be that the entire system is set to light theme because its day time? This should be overwritten for the app though.

You're right about the recent's in dark theme, I'll fix that and the artists headers tonight.

Edit: Updated PR with the above changes

@Twinklebear
Copy link
Copy Markdown
Collaborator

Let me know when the album description headers & artist headers are updated. I did also notice that when going to an artist page there's no loading feedback shown now, I'm not sure if these changes removed it (or maybe it was gone previously), but some loading feedback for users would be good as well. Otherwise it looks like the app is hanging/broken and showing a blank page.

@stuxo
Copy link
Copy Markdown
Contributor Author

stuxo commented Mar 20, 2016

Let me know when the album description headers & artist headers are updated.
This has been added
I did also notice that when going to an artist page there's no loading feedback shown now
Can you give me steps to reproduce this? I see a loading indicator when I do an artist search

@Twinklebear
Copy link
Copy Markdown
Collaborator

Ah so if you're directly to an artist page from the search we've already loaded them, it's a bit of an interesting thing in how the site handles searching for artists. If you search and match an artist name e.g. "the sword" the search response you get is actually the artist page, so no loading is done in the artist fragment, we just forward that data we got from the search response.

Try going to an artist from an album or similar artist list, (albums seem to be missing a loader too actually).

@stuxo
Copy link
Copy Markdown
Contributor Author

stuxo commented Mar 21, 2016

Oh ok, I can see it now. I'll have a quick look tonight and see if I did anything weird to break it.

Is that the last outstanding issue before we can close this PR?

@Twinklebear
Copy link
Copy Markdown
Collaborator

I think so, I might've missed a commit when I was noticing that the album description headers & artist description page headers weren't updated.

@stuxo
Copy link
Copy Markdown
Contributor Author

stuxo commented Mar 21, 2016

I have found the problem. It is because the app used to use a spinner in the AppBar for paged fragment activities. Since the migration to Toolbar, this hasn't been supported. That is why Artist and torrent group don't have indicators.

I won't have time to work on this for a while and I don't see a quick fix. I guess we can either park this PR, merge with know issue or you could have a look at it?

@Twinklebear
Copy link
Copy Markdown
Collaborator

Interesting, I thought there was also a big spinner shown in the fragment while it was loading. Yea I can take a look at this and see what happened to the loading spinner.

@Twinklebear
Copy link
Copy Markdown
Collaborator

Hey @stuxo , I checked the git blame on the ArtistFragment and it looks like we may have only ever shown progress in the menu bar, or a while back stopped showing the spinner in the fragment. So I don't think these changes removed any of that, and if you're ready to merge it I think everything looks good!

Adding back in some loading feedback to fragments missing it will have to be added as a todo.

@stuxo
Copy link
Copy Markdown
Contributor Author

stuxo commented Mar 29, 2016

I just pushed a fix for another missing background on dark theme. I feel like there may be other obscure things found once we roll it out. I'm happy to do a minor bug fix PR if a few things come back related to dark theme.

With that, I'm happy to merge.

@Twinklebear
Copy link
Copy Markdown
Collaborator

Cool, sounds good!

@Twinklebear Twinklebear merged commit 1b21c6f into Gwindow:develop Mar 29, 2016
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.

2 participants