Skip to content

Adjust pipeline-stage widths to accommodate more stages based on browser widths #987

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 6, 2016

Conversation

sg00dwin
Copy link
Member

@sg00dwin sg00dwin commented Dec 5, 2016

Fixes #925

Changes to allow space for more stages to fit inline before wrapping.

Current number of stages within single row before wrapping.

  • less than 992px - 2 stages
  • 992px to 1199px - 3 stages
  • more than 1200px - 4 stages

Proposed number of stages within single row before wrapping.

  • less than 992px - 2 stages
  • 992px to 1199px - 4 stages
  • 1200px to 1399px - 5 stages
  • 1400px to 1599px - 6 stages
  • more than 1600px - 7 stages

Stage widths are percentages. The least number of character spaces before the ellipsis is 20 and a title attribute is added to stage name and shown on hover.

4 stages when > 992px
screen shot 2016-12-02 at 4 38 40 pm

5 stages when > 1200px
screen shot 2016-12-02 at 4 38 04 pm

6 stages when > 1400px
screen shot 2016-12-02 at 4 37 28 pm

7 stages when > 1600px
screen shot 2016-12-02 at 4 37 08 pm

@sg00dwin
Copy link
Member Author

sg00dwin commented Dec 5, 2016

@jwforres @spadgett @rhamilto
thoughts on these pipeline stage width changes

@spadgett
Copy link
Member

spadgett commented Dec 5, 2016

I think it's better.

less than 992px - 2 stages

Do we still collapse down to one column with vertical arrows at mobile?

Copy link
Member

@rhamilto rhamilto left a comment

Choose a reason for hiding this comment

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

A couple of comments on the Less, but the sizing makes sense to me. 👍

@@ -145,8 +145,11 @@
text-align: center;
}
// hide arrow on last stage
&:last-child:before {
display: none;
&:last-child {
Copy link
Member

Choose a reason for hiding this comment

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

Do you feel strongly about this change? I prefer to avoid unnecessary nesting...

Copy link
Member Author

Choose a reason for hiding this comment

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

that actually wasn't meant to be included.

@@ -261,7 +264,7 @@
.pipeline .pipeline-stage {
padding-right: (@pipeline-padding * 5.2);
padding-bottom: (@pipeline-padding + 5);
width: 50%;
width: calc(100% / 2);
Copy link
Member

Choose a reason for hiding this comment

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

Curious: Why use calc() over letting the Less do the math?

Copy link
Member Author

Choose a reason for hiding this comment

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

no preference really, if it's better to use Less for the math, I'll change.

Copy link
Member

Choose a reason for hiding this comment

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

In this instance, I prefer to just let the Less do the math since the addition of calc doesn't gain us anything, but it's not a big deal either way.

Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer to use less for the math

@sg00dwin sg00dwin force-pushed the pipeline-stage-issue925 branch from faefb47 to 635658a Compare December 6, 2016 15:06
@sg00dwin
Copy link
Member Author

sg00dwin commented Dec 6, 2016

@spadgett
yes it still switches to a single column in mobile.
PR is updated and ready for merge.

@@ -146,7 +146,7 @@
}
// hide arrow on last stage
&:last-child:before {
display: none;
display: none;
Copy link
Member

Choose a reason for hiding this comment

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

nit: indentation

@sg00dwin sg00dwin force-pushed the pipeline-stage-issue925 branch from 635658a to d226fc5 Compare December 6, 2016 16:12
@spadgett
Copy link
Member

spadgett commented Dec 6, 2016

[merge]

@spadgett
Copy link
Member

spadgett commented Dec 6, 2016

Error: Timed out waiting for the WebDriver server at http://172.18.1.112:60902/wd/hub

[merge]

@openshift-bot
Copy link

Evaluated for origin web console merge up to d226fc5

@openshift-bot
Copy link

openshift-bot commented Dec 6, 2016

Origin Web Console Merge Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_web_console/811/) (Base Commit: 99070c1)

@openshift-bot openshift-bot merged commit f697f50 into openshift:master Dec 6, 2016
@sg00dwin sg00dwin deleted the pipeline-stage-issue925 branch December 6, 2016 17:49
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.

4 participants