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

Fix toggle link positioning for truncation directive #1316

Merged
merged 1 commit into from
Mar 7, 2017

Conversation

sg00dwin
Copy link
Member

@sg00dwin sg00dwin commented Mar 1, 2017

Fixes #1277

screen shot 2017-03-01 at 1 52 37 pm

@@ -4,15 +4,15 @@
<span ng-attr-title="{{content}}">
<span ng-bind-html="truncatedContent | highlightKeywords : keywords" class="truncated-content"></span>&hellip;
</span>
<a ng-if="expandable" href="" ng-click="toggles.expanded = true" style="margin-left: 5px; white-space: nowrap;">See all</a>
<a ng-if="expandable" href="" ng-click="toggles.expanded = true" class="truncated-content-toggle-link">See all</a>
Copy link
Member

@spadgett spadgett Mar 1, 2017

Choose a reason for hiding this comment

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

If "See all" wraps by itself onto the next line, is the left margin wrong?

@sg00dwin sg00dwin force-pushed the collapse-link-issue1277 branch from 0a87004 to 5dec7f7 Compare March 2, 2017 17:36
@sg00dwin
Copy link
Member Author

sg00dwin commented Mar 2, 2017

@spadgett updated pr so margin is applied around content in collapsed state so it aligns if wrapped

screen shot 2017-03-02 at 12 30 49 pm

screen shot 2017-03-02 at 12 30 11 pm

Copy link
Member

@spadgett spadgett left a comment

Choose a reason for hiding this comment

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

Just a couple nits, otherwise LGTM

margin-right: 10px;
}

.truncated-content-toggle-link {
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 just call this .truncation-collapse-link since it is no longer for the see all link.

<span ng-bind-html="truncatedContent | highlightKeywords : keywords" class="truncated-content"></span>&hellip;
</span>
<a ng-if="expandable" href="" ng-click="toggles.expanded = true" style="margin-left: 5px; white-space: nowrap;">See all</a>
<a ng-if="expandable" href="" ng-click="toggles.expanded = true" class="nowrap">See all</a>
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 mind changing to headline case "See All" while you're in the code?

@spadgett spadgett self-assigned this Mar 7, 2017
@sg00dwin sg00dwin force-pushed the collapse-link-issue1277 branch from 5dec7f7 to 0140d25 Compare March 7, 2017 15:25
@sg00dwin
Copy link
Member Author

sg00dwin commented Mar 7, 2017

@spadgett pr has been updated ptal

Copy link
Member

@spadgett spadgett left a comment

Choose a reason for hiding this comment

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

Thanks @sg00dwin

@spadgett
Copy link
Member

spadgett commented Mar 7, 2017

[merge]

@spadgett
Copy link
Member

spadgett commented Mar 7, 2017

Merge failed from dist mismatch

@sg00dwin sg00dwin force-pushed the collapse-link-issue1277 branch from 0140d25 to 19fa849 Compare March 7, 2017 17:26
@sg00dwin
Copy link
Member Author

sg00dwin commented Mar 7, 2017

pushed an updated main.css. Lets see if that fixes it.

@spadgett
Copy link
Member

spadgett commented Mar 7, 2017

[merge]

@openshift-bot
Copy link

Evaluated for origin web console merge up to 19fa849

@openshift-bot
Copy link

openshift-bot commented Mar 7, 2017

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

@openshift-bot openshift-bot merged commit f11890b into openshift:master Mar 7, 2017
@sg00dwin sg00dwin deleted the collapse-link-issue1277 branch March 7, 2017 21:04
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.

3 participants