-
Notifications
You must be signed in to change notification settings - Fork 60
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
Changing overlay layout to a single column #413
Conversation
rhamilto
commented
Sep 1, 2017
•
edited
Loading
edited
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @rhamilto. Nice to see us remove more code than we're adding.
<div class="order-service-details"> | ||
<div class="order-service-details-top"> | ||
<div class="service-icon"> | ||
<span class="icon {{$ctrl.imageStream.iconClass}}"></span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aria-hidden="true"
private showInfo = () => { | ||
this.clearValidityWatcher(); | ||
this.ctrl.nextTitle = 'Next >'; | ||
this.reviewStep.allowed = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line looks wrong. I think we can just delete it.
@@ -1,6 +1,6 @@ | |||
<div class="order-service"> | |||
<pf-wizard | |||
hide-header="true" | |||
title="{{$ctrl.imageStream.name}} {{$ctrl.istag.name}}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you check that this updates correctly when you pick different versions in the dropdown inside the wizard?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed that it does. :)
<div class="order-service-details"> | ||
<div class="order-service-details-top"> | ||
<div class="service-icon"> | ||
<span ng-if="!$ctrl.imageUrl" class="icon {{$ctrl.iconClass}}"></span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aria-hidden="true"
</div> | ||
<div ng-if="$ctrl.serviceClass.resource.externalMetadata.documentationUrl" class="order-service-documentation-url"> | ||
<a ng-href="{{$ctrl.serviceClass.resource.externalMetadata.documentationUrl}}" target="_blank" class="learn-more-link">Learn More <i class="fa fa-external-link" aria-hidden="true"></i></a> | ||
</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably add externalMetadata.supportUrl
too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@spadgett's comments addressed. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks @rhamilto
We might want to wait for openshift/origin-web-console#2127 to merge this so that all changes are ready to go together
@jeff-phillips-18, can you take a look today? @spadgett is hoping we can this merged so we can also merge openshift/origin-web-console#2127 today. |
@rhamilto yeah, catching up with reviews now. Sorry for the delay. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍