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

Addition of truncate class to tile headers and when no deployments have started #1188

Merged
merged 1 commit into from
Feb 24, 2017
Merged

Addition of truncate class to tile headers and when no deployments have started #1188

merged 1 commit into from
Feb 24, 2017

Conversation

sg00dwin
Copy link
Member

Include ng-class to selectively apply margin-right for tiles that have rc shield & number
Tested in FF, Chrome, Safari, IE

Fixes #1175

screen shot 2017-01-27 at 3 07 00 pm

@sg00dwin
Copy link
Member Author

@rhamilto @spadgett

spadgett
spadgett requested changes Jan 27, 2017
View reviewed changes
app/views/overview/_dc.html
@@ -1,8 +1,8 @@
<div class="overview-tile" ng-class="{ 'deployment-in-progress': inProgressDeployment }">
<ng-include src="'views/overview/_service-header.html'"></ng-include>
<div class="overview-tile-header">
<div class="rc-header">
<div>
<div class="rc-header" ng-class="{ 'rc-header-shield' : activeReplicationController}">
Copy link
Member

@spadgett spadgett Jan 27, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

You'll need to add the rc-header-shield class to the header in _deployment.html as well.

@rhamilto
Copy link
Member

Service names also have the same problem. Want to fix that here? Or another issue?

@sg00dwin sg00dwin force-pushed the overview-long-strings1175 branch from 563f84c to 59b2936 Compare January 30, 2017 19:31
@sg00dwin
Copy link
Member Author

@rhamilto @spadgett
Added rc-header-shield to _deployment.html and service names are now truncated also.

Tested in FF, Chrome, Safari, IE

screen shot 2017-01-30 at 2 18 01 pm

spadgett
spadgett requested changes Jan 30, 2017
View reviewed changes
app/views/overview/_deployment.html Outdated
@@ -1,8 +1,8 @@
<div class="overview-tile" ng-class="{ 'deployment-in-progress': inProgressDeployment }">
<ng-include src="'views/overview/_service-header.html'"></ng-include>
<div class="overview-tile-header">
<div class="rc-header">
<div>
<div class="rc-header" ng-class="{ 'rc-header-shield' : activeReplicationController}">
Copy link
Member

Choose a reason for hiding this comment

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

<div class="rc-header" ng-class="{ 'rc-header-shield': latestReplicaSet && latestRevision && !inProgressDeployment }">

Copy link
Member

Choose a reason for hiding this comment

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

@sg00dwin You can test with

https://github.com/openshift/origin-web-console/blob/master/test/testdata/k8s-deployments.yaml

@spadgett spadgett self-assigned this Jan 31, 2017
@sg00dwin sg00dwin force-pushed the overview-long-strings1175 branch 2 times, most recently from da5310c to 34ab3dd Compare January 31, 2017 22:00
@spadgett spadgett added this to the 1.5.0 milestone Feb 1, 2017
@spadgett
Copy link
Member

spadgett commented Feb 1, 2017

@sg00dwin I see you pushed some updates. Ready for another review?

@sg00dwin sg00dwin force-pushed the overview-long-strings1175 branch from 34ab3dd to 5e7583e Compare February 1, 2017 16:55
@sg00dwin
Copy link
Member Author

sg00dwin commented Feb 1, 2017

@spadgett Yes please review.

I enabled the truncation of service names. To get that to work, required some changes to overview-service and .service-name and removing the flex from tiles for .no-deployments-message and then applied the overview-tile rules, so that they wouldn't over expand. Also, the truncation along with line-height: 1.1 inherited from .h3() caused clipping of text descenders. So I increased line-height and offset the additional height by reducing the top and bottom margins so it maintained it's current height.

Additionally
Since the links in .empty-tile and .empty-dc where in the middle of a sentence I couldn't truncate so I applied .word-break-all so that they would wrap in every browser. Since this can cause weird breaking mid word, I reduced the left and right padding so that it would allow more room for long strings and reduce the chance of it occurring.

screen shot 2017-01-31 at 4 55 06 pm

screen shot 2017-01-31 at 4 52 45 pm

spadgett
spadgett reviewed Feb 2, 2017
View reviewed changes
app/views/overview/_service.html Outdated
<ng-include src="'views/overview/_service-header.html'"></ng-include>
<div class="empty-tile">
<h2>No deployments or pods.</h2>
<p>
Service
<a ng-href="{{service | navigateResourceURL}}">{{service.metadata.name}}</a>
<div class="word-break-all">Service <a ng-href="{{service | navigateResourceURL}}">{{service.metadata.name}}</a></div>
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this going to add a line break in the middle of a sentence?

Copy link
Member Author

Choose a reason for hiding this comment

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

yep, these will be fixed.

app/views/overview/_service-group.html Outdated
@@ -71,12 +71,10 @@ <h3 class="route-title truncate">
<div class="no-child-services-message">
<div class="empty-tile">
<h2>No grouped services.</h2>
<p>
No services are grouped with <a ng-href="{{service | navigateResourceURL}}">{{service.metadata.name}}</a>.
No services are grouped with <div class="word-break-all"><a ng-href="{{service | navigateResourceURL}}">{{service.metadata.name}}</a>.</div>
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this going to add a line break in the middle of a sentence?

app/views/overview/_dc.html Outdated
@@ -43,8 +43,8 @@
A new deployment will start automatically when
<span ng-if="imageChangeTriggers.length === 1">
an image is available for
<a ng-href="{{urlForImageChangeTrigger(imageChangeTriggers[0], deploymentConfig)}}">
{{imageChangeTriggers[0].imageChangeParams.from | imageObjectRef : deploymentConfig.metadata.namespace}}</a>.
<div class="truncate mar-bottom-xl"><a ng-href="{{urlForImageChangeTrigger(imageChangeTriggers[0], deploymentConfig)}}">
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this going to add a line break in the middle of a sentence?

@sg00dwin sg00dwin force-pushed the overview-long-strings1175 branch 2 times, most recently from eaabe8b to e585998 Compare February 2, 2017 21:00
@sg00dwin
Copy link
Member Author

sg00dwin commented Feb 2, 2017

@spadgett PTAL

@sg00dwin
Copy link
Member Author

sg00dwin commented Feb 2, 2017

screen shot 2017-02-02 at 3 36 29 pm

@spadgett
Copy link
Member

spadgett commented Feb 3, 2017

This change breaks sentences mid-word in Firefox. Can we use word-break instead of word-break-all?

openshift_web_console

@sg00dwin
Copy link
Member Author

sg00dwin commented Feb 6, 2017

With just <p class="word-break">... There would be scenarios where the long string wouldn't break and cause the tile width to expand, causing it to bump down below it's sibling tile.

screen shot 2017-02-06 at 10 43 22 am

@sg00dwin
Copy link
Member Author

sg00dwin commented Feb 6, 2017

I'm looking further to see if we can incorporate hyphens with our word-break rule

@sg00dwin sg00dwin force-pushed the overview-long-strings1175 branch from e585998 to 6e39565 Compare February 6, 2017 20:56
@sg00dwin
Copy link
Member Author

sg00dwin commented Feb 6, 2017

@spadgett I have .break-word working in those empty tiles, ftw!
Cross browser screenshots...

screen shot 2017-02-06 at 3 52 21 pm

screen shot 2017-02-06 at 3 51 43 pm

screen shot 2017-02-06 at 3 46 39 pm

screen shot 2017-02-06 at 3 44 57 pm

@sg00dwin sg00dwin force-pushed the overview-long-strings1175 branch from 6e39565 to bfda9c4 Compare February 9, 2017 14:24
@spadgett spadgett modified the milestones: 1.6.0, 1.5.0 Feb 10, 2017
@rhamilto
Copy link
Member

#1205 is a related bug that can also be fixed in this PR.

@spadgett
Copy link
Member

@sg00dwin Approved, but needs rebase

@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 21, 2017
@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 21, 2017
@sg00dwin sg00dwin force-pushed the overview-long-strings1175 branch from bfda9c4 to c8905b9 Compare February 23, 2017 19:08
@sg00dwin
Copy link
Member Author

@spadgett this is now rebased

@sg00dwin
Addition of truncate class to tile headers and when no deployments ha…
901d64f 
…ve started.

And add word-break to empty-dc and empty-rc
Fixes openshift#1175
@sg00dwin sg00dwin force-pushed the overview-long-strings1175 branch from c8905b9 to 901d64f Compare February 24, 2017 17:53
@sg00dwin
Copy link
Member Author

@spadgett removed vendor.js

@spadgett
Copy link
Member

[merge]

@openshift-bot
Copy link

Evaluated for origin web console merge up to 901d64f

@openshift-bot
Copy link

openshift-bot commented Feb 24, 2017 •
edited
Loading

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

@openshift-bot openshift-bot merged commit a05779d into openshift:master Feb 24, 2017
@sg00dwin sg00dwin deleted the overview-long-strings1175 branch February 27, 2017 14:24
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

Assignees

@spadgett spadgett

Labels
None yet
Projects
None yet
Milestone
3.6.0
Development

Successfully merging this pull request may close these issues.
4 participants
@sg00dwin @rhamilto @spadgett @openshift-bot