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

Material buttons and checkboxes #1090

Merged
merged 63 commits into from
Oct 22, 2015
Merged

Material buttons and checkboxes #1090

merged 63 commits into from
Oct 22, 2015

Conversation

AndyScherzinger
Copy link
Contributor

Started a followup branch material_buttons and PR to #1076 as per discussion with @davivel

Things being implemented on this branch (already):

  • material design buttons --> DONE
  • material design check boxes on multi-select lists --> DONE

Things I would start working on (subject to discussion):

pinging @davivel @jancborchardt @tobiasKaminsky @masensio for the discussion

@AndyScherzinger
Copy link
Contributor Author

First screenshots for discussion especially with @jancborchardt

button on login screen
device-2015-08-07-181858

multi select check boxes and buttons (on lollipop device)
device-2015-08-07-180140

multi select check boxes and buttons (on Android 15 device) --> See the problem with not having a shadow on the button
device-2015-08-07-181726

@jancborchardt
Copy link
Member

@AndyScherzinger wow, you're awesome! :)
The last screenshot is strange as you say – also the triangle in the top bar has a whole bunch of whitespace.

@AndyScherzinger
Copy link
Contributor Author

The whitespace is fine, this also happens on lollipop - it depends on the longest path name in the chooser, thus the size varies per folder :)

@jancborchardt
Copy link
Member

@AndyScherzinger that’s kind of strange though, no? It seems because the dropdown arrow is floating (unlike for a dropdown box) it should always stick next to the text.

@AndyScherzinger
Copy link
Contributor Author

Yeah, I guesss. It still seems to be the Google desired (?!) behaviors...

@AndyScherzinger
Copy link
Contributor Author

@purigarcia can you check the button's look and feel for Android 4.1/4.0 on a device? My screenshots for pre lollipop are done in an emulator, so I am not sure a "real" device would render them the exact same way.

On the other hand, the contrast is very low (light background color and light button coloring), @jancborchardt any opinion on that? Maybay make the buttons blue (like the action bar?). This would solve the rendering issue (in a pragmatic way)

@jancborchardt
Copy link
Member

@AndyScherzinger I would definitely not make both buttons dark blue. Maybe the primary button (in this case »Upload«) should be differently colored – try #35537a (same as the highlight color, also what we use in the web interface for primary buttons).

@AndyScherzinger
Copy link
Contributor Author

@jancborchardt see attached the primary button in blue with the secondary one in grey or white

_Lollipop 5.1:_
device-2015-08-12-155831
or
device-2015-08-12-161306

_Android 4.0:_
device-2015-08-12-155932_android15
or
device-2015-08-12-161803_android15

@jancborchardt
Copy link
Member

@AndyScherzinger nice! I’d say the first version of each is better, with the other button being grey rather than white.

@AndyScherzinger
Copy link
Contributor Author

_Fixed Button coloring_
device-2015-08-12-183505

@jancborchardt
Copy link
Member

Great! :) 👍

@AndyScherzinger
Copy link
Contributor Author

For some reason I got the highlight color of the textfield fixed on this branch (more styling done on here already), but it doesn't work on the material_toolbar branch (yet) :( - So maybe leave it on here and take this PR to be a fix for the bug on PR #1076 ?

device-2015-08-12-190042

…into material_buttons

Conflicts:
	res/layout/ssl_untrusted_cert_layout.xml
	res/layout/ssl_validator_layout.xml
@AndyScherzinger AndyScherzinger mentioned this pull request Aug 13, 2015
@AndyScherzinger
Copy link
Contributor Author

PR is ready for Test imho, what do you think @jancborchardt @purigarcia ?

@jancborchardt
Copy link
Member

I think it’s awesome, as I said 👍 :)

@AndyScherzinger
Copy link
Contributor Author

@jancborchardt Danke für die Blumen :) - see attached the standard folder view. It is a minor change but a change nevertheless. According to the material Guidelines I changed the standard item padding to 16dp and the icon size in the list items to 32dp. This adds up to slightly more whitespace (which is good imho) but I something I like to mention just to make sure this change isn't missed in the review and any feedback has been considered.

device-2015-08-13-173827

@tobiasKaminsky
Copy link
Contributor

Code looks fine to me, a quick test in an emulator (nexus 5) also looks fine 👍

@AndyScherzinger
Copy link
Contributor Author

Great! Thanks for the code review @tobiasKaminsky

Is this PR fine design wise @jancborchardt
Is this PR fine code wise @davivel @masensio

@jancborchardt
Copy link
Member

👍 design wise as said before. :)

@AndyScherzinger
Copy link
Contributor Author

Sweet, so code review from the ownCloud Team would be next I guess, @davivel @rperezb @masensio

BTW: Could be create a new label for this kind of task, as in Ready4CodeReview or something like that?

@AndyScherzinger AndyScherzinger mentioned this pull request Oct 11, 2015
4 tasks
@tobiasKaminsky
Copy link
Contributor

BTW: Could be create a new label for this kind of task, as in Ready4CodeReview or something like that?

See #1130, there I suggested this already.

@AndyScherzinger
Copy link
Contributor Author

👍

@rperezb
Copy link

rperezb commented Oct 13, 2015

@AndyScherzinger, yes I know, this is very appreciated! it´s on the backlog.
Currently we are focus on share with users: #880
Developing this feature, @masensio detected that it would be great to have included this pr: #1065 and I'm about the check it
Besides, it would be active on our end soon this one: #1109
Having said, yes, we will check it...not sure when, sorry

@rperezb rperezb added this to the backlog milestone Oct 15, 2015
@AndyScherzinger AndyScherzinger merged commit d64946f into master Oct 22, 2015
@AndyScherzinger
Copy link
Contributor Author

Aaaaah! @davivel I made a big mistake and pushed this branch to master instead of the other way round. Are you able to revert my push/commit in any way? I'm very Sorry!!!!

@davivel
Copy link
Contributor

davivel commented Oct 22, 2015

Don't worry, I'll revert it.

@davivel
Copy link
Contributor

davivel commented Oct 22, 2015

Master fixed, but the PR is not recovered by Github. New PR, same branch in #1209

@AndyScherzinger
Copy link
Contributor Author

Thanks a lot @davivel !!!! 👍

masensio pushed a commit that referenced this pull request Dec 29, 2015
@AndyScherzinger AndyScherzinger modified the milestones: 1.9.1-current, backlog Dec 29, 2015
@AndyScherzinger
Copy link
Contributor Author

Just for documentation: this has been merged to master via #1209 now

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

Successfully merging this pull request may close these issues.

7 participants