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.

Sign up for GitHub

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

Jump to bottom

Fix readonly parameters margins #518

Merged
merged 1 commit into from
Oct 20, 2017
Merged

Fix readonly parameters margins #518

merged 1 commit into from
Oct 20, 2017

Conversation

jeff-phillips-18
Copy link
Member

Also fixes next button title going back to the plan page of the update service wizard.

@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 20, 2017
spadgett
spadgett requested changes Oct 20, 2017
View reviewed changes
dist/less/catalogParameters.less Outdated
@@ -8,6 +8,19 @@
padding-left: 0;
}
&.readonly {
.control-label {
padding-right: 0;
word-break: break-word;
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is a standard word-break value. Do you mean word-wrap: break-word;?

https://developer.mozilla.org/en-US/docs/Web/CSS/word-break

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, sorry.

@spadgett
Copy link
Member

@jeff-phillips-18 Do you mind pasting a screenshot?

@jeff-phillips-18
Fix readonly parameters margins
e69e2e2 
Also fixes next button title going back to the plan page of the update
service wizard.
@jeff-phillips-18
Copy link
Member Author

image

@spadgett
Copy link
Member

I feel like the parameters need some top margin. It seems crowded.

Should we reuse the same font-style as the overview for the "PARAMETERS" header? It looks a bit bigger here.

The left indentation looks strange to me still.

@jwforres @beanh66 opinion?

@jwforres
Copy link
Member

+1 to needing some top margin
+1 to indention looking strange

@beanh66
Copy link
Member

beanh66 commented Oct 20, 2017

@spadgett After seeing this, agreed all around.

+1 to adding more space above PARAMETERS
+1 to left aligning, I think we will be fine without the indent since we have the PARAMETERS header
+1 the header should be the same font we use on the overview for things like DEPLOYMENT

@jeff-phillips-18
Copy link
Member Author

Those changes:
image

@jeff-phillips-18
Copy link
Member Author

For what it's worth, that styling is all in console and will not be part of this PR.

@serenamarie125
Copy link

PARAMETERS looks to be a smaller font than Reveal Values now @spadgett @jeff-phillips-18

@jeff-phillips-18
Copy link
Member Author

Yes, 11px vs 13px

@jeff-phillips-18
Copy link
Member Author

A few more tweaks (again not in this PR):
image

@serenamarie125
Copy link

not as noticeable now that you've changed the spacing. Looks better to me!

@spadgett spadgett merged commit b53588a into openshift:master Oct 20, 2017
@jeff-phillips-18 jeff-phillips-18 mentioned this pull request Oct 23, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@spadgett spadgett spadgett approved these changes

@dtaylor113 dtaylor113 Awaiting requested review from dtaylor113

Assignees
No one assigned
Labels
size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@jeff-phillips-18 @spadgett @jwforres @beanh66 @serenamarie125 @openshift-ci-robot