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

Truncation #221

Merged
Merged

Conversation

LotteHofstede
Copy link
Contributor

I implemented a truncation component that can contain one or more "truncatable" parts.
The truncation component itself can have one of the following 2 state: collapsed or expanded.
When the component is expanded, all its child "truncatable" parts are either totally expanded or capped at a specified amount of lines (@input() max-lines).
When the component is collapsed, all its child "truncatable' parts are capped at a specified amount of lines (@input() min-lines).

I tried a lot of different approaches to implement the multiline clamping, including libraries like shave, ellipsis, clamp...
The issue with all these libraries is that heights can only be calculated when the content is already displayed. At the moment, there's no way of knowing when a component is totally loaded, the default Angular Lifecycle Hooks can't provide us with a solution.

This is why @artlowel and I chose to implement this with CSS instead. We took some inspiration for this from popular websites with a comparable layout. The text is now fading out at the end of a clamped line.

You might expect an ellipsis at the end of a clamped line instead, but with CSS this is only possible outside of a text block. This did not give us the expected results, especially in list views.

For now, the truncation has been applied to the search result items (both list and grid view). Clicking a truncated component, will expand it, clicking it again will collapse it again.

@ghost ghost assigned LotteHofstede Feb 8, 2018
@ghost ghost added the needs review label Feb 8, 2018
…ncation-implementation-test-fixes

Conflicts:
	src/app/shared/object-grid/search-result-grid-element/community-search-result/community-search-result-grid-element.component.spec.ts
Conflicts:
	src/app/shared/object-grid/collection-grid-element/collection-grid-element.component.spec.ts
	src/app/shared/object-grid/community-grid-element/community-grid-element.component.spec.ts
	src/app/shared/object-grid/item-grid-element/item-grid-element.component.spec.ts
	src/app/shared/object-grid/search-result-grid-element/collection-search-result/collection-search-result-grid-element.component.spec.ts
	src/app/shared/object-grid/search-result-grid-element/community-search-result/community-search-result-grid-element.component.spec.ts
	src/app/shared/object-grid/search-result-grid-element/item-search-result/item-search-result-grid-element.component.spec.ts
Copy link
Member

@tdonohue tdonohue left a comment

Choose a reason for hiding this comment

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

👍 Gave this a test today, and it seems to work well. The code looks fine to me too, no objections.

I think this is ready to merge. Although, with master currently broken (see #232), I'm going to wait to push the green button until we can fix master.

@PTrottier
Copy link

Works visually very well on Fedora 27 in Firefox 58.0.1.
I noticed that in Chrome (64.0.3282.140), when trying to select the truncated text, the behaviour of the selection differs from what a user normally expects. After you start to select the text, when you reach the point of truncation, your selection jumps back and starts over at the beginning.

@atarix83
Copy link
Contributor

atarix83 commented Mar 5, 2018

It looks good to me too

@tdonohue
Copy link
Member

tdonohue commented Mar 5, 2018

I've retested this today, and I do see the same behavior as @PTrottier around selecting text in Chrome. However, I noticed this only occurs if the text is faded/truncated. If you click on the text, it appears fully and you can easily select it. But, if the fade is in place, selecting the faded text is harder to achieve.

That said, I think this is a relatively minor UX issue (and, as I said, if you click to expand the text first, it can be easily selected). So, I think this PR should still be merged as-is, and we can always open up a new ticket to describe this minor UX issue, as necessary.

@PTrottier
Copy link

Agreed, this behaviour also does not occur in Firefox.

@tdonohue
Copy link
Member

tdonohue commented Mar 5, 2018

Merged, and logged the minor UX bug here: #234

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