Skip to content

Add log in style #37

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

Merged
merged 1 commit into from
Dec 14, 2017
Merged

Add log in style #37

merged 1 commit into from
Dec 14, 2017

Conversation

rhamilto
Copy link
Member

@rhamilto rhamilto commented Dec 1, 2017

Need to test to ensure it works as expected. @jwforres suggested @enj
may be able to instruct me on how to do so.

screenshots of the straight html
screen shot 2017-12-11 at 5 18 49 pm

localhost_8080_error html

@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 1, 2017
@mrogers950
Copy link

@rhamilto cool! you can make a test deployment using oauth-proxy/contrib/sidecar.yaml , just change the oauth-proxy spec to point to your test docker repo/image. This will not show the custom login though, to see that you would also need to add -htpasswd-file=/path/to/htpasswdfile to the oauth-proxy args and a configMap with the htpasswd contents.

@rhamilto
Copy link
Member Author

rhamilto commented Dec 7, 2017

Thanks, @mrogers950. Given my lack of familiarity with this repo, is it possible you or someone else could test my changes? I'm having a hard time deciphering how to do so.

@mrogers950
Copy link

@rhamilto Sure, I tested your changes and the design looks as it should (including the htpasswd login box). One gripe is maybe the login boxes should be closer together when they are both displayed.
login

@rhamilto
Copy link
Member Author

Fantastic. Thanks, @mrogers950! Both log in boxes should not be displayed, so we've got a bug. I see the issue. Let me fix.

@mrogers950
Copy link

@rhamilto The previous template shows both boxes. I think we want to keep that behavior as you can log in with either an OpenShift login or directly with the htpasswd contents when configured as such.

Need to test to ensure it works as expected.  @jwforres suggested @enj
may be able to instruct me on how to do so.
@rhamilto rhamilto changed the title [WIP] Add log in style Add log in style Dec 11, 2017
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 11, 2017
@rhamilto
Copy link
Member Author

Spacing bug addressed. Thanks, @mrogers950!

@jwforres
Copy link
Member

@mrogers950 need anything else or is this good to merge?

@mrogers950
Copy link

@jwforres @rhamilto I'm wondering if this this page will display properly on a mobile screen.. Is it possible to confirm there will not be any issues on mobile?

@rhamilto
Copy link
Member Author

@jwforres @rhamilto I'm wondering if this this page will display properly on a mobile screen.. Is it possible to confirm there will not be any issues on mobile?

The page is fully responsive and adapts to the size of the screen. Here are a couple of quick screenshots for reference.

iPhone 5
_users_rhamilto_desktop_log-in html iphone 5

iPhone 6Plus
_users_rhamilto_desktop_log-in html iphone 6 plus

@mrogers950
Copy link

@rhamilto perfect, thanks!

@mrogers950 mrogers950 merged commit 42400a5 into openshift:master Dec 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants