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

Authorization screen #16

Merged
merged 11 commits into from Jun 19, 2015
Merged

Authorization screen #16

merged 11 commits into from Jun 19, 2015

Conversation

arturdryomov
Copy link
Contributor

This time I tried to pay attention to details and follow mockups as much as possible, so nitpicking is very welcome.

The Hawkular authorization is now required and the authorization screen will show up when it is needed. If the authorization process failed the application will quit, because without it you cannot do anything without fancy crashes.

BTW. After capturing screenshots red buttons seem extremely ugly. Should I change them to default grey?

device-2015-06-17-155031
device-2015-06-17-155145
device-2015-06-17-155123
device-2015-06-17-155135
device-2015-06-17-155106

@arturdryomov
Copy link
Contributor Author

@pilhuhn @danielpassos Just added environment support as afterthought. It will help to be just a little more flexible.

Also, there are some screenshots of buttons to help you to choose. Probably I should change it to grey — there is enough red already.

device-2015-06-17-205651
device-2015-06-17-205803

@arturdryomov arturdryomov changed the title Authorization Screen Authorization screen Jun 17, 2015
@danielpassos
Copy link
Member

I think we not need worry about the colors for now


private void setUpDefaults() {
if (Android.isDebugging()) {
hostEdit.append(BackendEndpoints.Community.HOST);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You was the first guy I see use append to set a text instead of use setText 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don’t remember reasoning behind this honestly, but probably it was related to some bug in the past I encountered. Maybe some focus stuff. I’ll change it.

@danielpassos
Copy link
Member

I notice you are using for build some object:

  • SomeClass.of(...)
  • SomeClass.Builder.of(...)
  • SomeClass.ofFoo(...)

Let's try to make it concise

@danielpassos
Copy link
Member

Could you rebase it?

@danielpassos
Copy link
Member

I'm trying to run it but I'm getting:

06-17 21:03:58.970    8263-8263/org.hawkular.client.android D/AuthorizationActivity﹕ Authorization failed. AuthorizationException{error=invalid_grant}

@arturdryomov
Copy link
Contributor Author

Updated and rebased.

@danielpassos
Copy link
Member

Only to be clear my comment about append instead of setText not was a call for change just a curiosity

@arturdryomov
Copy link
Contributor Author

Only to be clear my comment about append instead of setText not was a call for change just a curiosity

I understand, I just cannot remember any valid arguments in favor of append.

@arturdryomov
Copy link
Contributor Author

@danielpassos What is left to get this PR merged?

@arturdryomov arturdryomov mentioned this pull request Jun 19, 2015
@danielpassos
Copy link
Member

Only I have time to merge :P

@danielpassos danielpassos merged commit 0a166b6 into hawkular:master Jun 19, 2015
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