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.

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

Show 'none' when there is no builder image #6366

Merged

Conversation

rhcarvalho
Copy link
Contributor

Using 'truncate' when the tag is empty causes the next <dd> contents to be shifted up, and the labels will point to the wrong piece of information.
Furthermore, we already show 'none' in multiple other places in the Web Console in similar contexts.

Fixes #6364.

@rhcarvalho
Copy link
Contributor Author

@jwforres PTAL

@spadgett
Copy link
Member

May want to use directive truncate-long-text instead if we're worried about long values.

@spadgett
Copy link
Member

Does this only occur with empty values? Does adding &nbsp; in place of the empty value fix it?

@rhcarvalho
Copy link
Contributor Author

@spadgett how will that behave in the absence of a value?

@spadgett
Copy link
Member

how will that behave in the absence of a value?

truncate-long-text? It should just be empty, but it doesn't use the problematic truncate class.

@spadgett
Copy link
Member

Another possible fix is to use ng-class to only add truncate when there is content.

https://docs.angularjs.org/api/ng/directive/ngClass

@rhcarvalho
Copy link
Contributor Author

truncate-long-text works and seems to be a better fix IMO. Updated the PR. Thanks @spadgett

@spadgett
Copy link
Member

Sorry, I wasn't clear. It's a directive, not a class. So you'd need to use it this way:

<truncate-long-text content="(build | buildStrategy).from | imageObjectRef : build.metadata.namespace" limit="40"></truncate-long-text>

@rhcarvalho
Copy link
Contributor Author

Actually truncate-long-text has a draw back...

selection_009

Is splits the text into multiple lines, while truncate truncates and appends "..."

@rhcarvalho
Copy link
Contributor Author

limit="40"

Why 40?

@spadgett
Copy link
Member

truncate-long-text will truncate at whatever character limit you specify. truncate class will use the available space, so it might be better.

I'm not sure 40 is the value we want, just an example.

@php-coder
Copy link
Contributor

WDYT about showing default value instead of empty string? Like Builder image: none. Or maybe even don't show this field at all (by using ng-if)?

@rhcarvalho
Copy link
Contributor Author

Talked on IRC with @spadgett and @php-coder and we decided on showing none as we have in multiple other places in the Web Console.

Using 'truncate' when the tag is empty causes the next <dd> contents to
be shifted up, and the labels will point to the wrong piece of
information.
Furthermore, we already show 'none' in multiple other places in the Web
Console in similar contexts.
@rhcarvalho
Copy link
Contributor Author

@spadgett PTAL

@rhcarvalho rhcarvalho changed the title Do not use class truncate Show 'none' when there is no builder image Dec 17, 2015
@rhcarvalho
Copy link
Contributor Author

@jwforres PTAL

@php-coder
Copy link
Contributor

I would rather use custom filter for that instead of copy&paste <span><em>none<em></span>. The example of such filter could be found on StackOverflow: http://stackoverflow.com/questions/27490652/angular-default-value-if-binding-is-null

@rhcarvalho
Copy link
Contributor Author

Grepping <em>none</em></span> finds 14 occurrences. That's a nice refactor, but not for me / this PR. I'd keep this focused on fixing the bug.

@spadgett
Copy link
Member

Not sure if we can have the em markup in the none case using a default value filter. Often, we just say

{{ foo || "none" }}

@jwforres
Copy link
Member

agree with @rhcarvalho not the PR for a refactor, this is fine [merge]

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/4399/) (Image: devenv-rhel7_2984)

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 7259c16

@openshift-bot
Copy link
Contributor

[Test]ing while waiting on the merge queue

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 7259c16

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/7928/)

@rhcarvalho
Copy link
Contributor Author

Jenkins test failure was a flake: #6381

openshift-bot pushed a commit that referenced this pull request Dec 18, 2015
@openshift-bot openshift-bot merged commit 30ee222 into openshift:master Dec 18, 2015
@rhcarvalho rhcarvalho deleted the webconsole-build-config-info branch December 18, 2015 09:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants