Skip to content

TruncatableComponent usability improvement#1619

Merged
tdonohue merged 18 commits intoDSpace:mainfrom
4Science:DSC-516-Style-New
Jun 13, 2022
Merged

TruncatableComponent usability improvement#1619
tdonohue merged 18 commits intoDSpace:mainfrom
4Science:DSC-516-Style-New

Conversation

@atarix83
Copy link
Copy Markdown
Contributor

References

Description

This PR has the aim to improve the usability issues described in #1551

Instructions for Reviewers

To review this PR browse pages where the TruncatableComponent is used. It's usualy used for search result pages or item pages

List of changes in this PR:

  • An additional button "Show more" it's been added after the gradient effect
    Schermata da 2022-04-27 17-47-16

Checklist

  • My PR is small in size (e.g. less than 1,000 lines of code, not including comments & specs/tests), or I have provided reasons as to why that's not possible.
  • My PR passes TSLint validation using yarn run lint
  • My PR doesn't introduce circular dependencies
  • My PR includes TypeDoc comments for all new (or modified) public methods and classes. It also includes TypeDoc for large or complex private methods.
  • My PR passes all specs/tests and includes new/updated specs or tests based on the Code Testing Guide.
  • If my PR includes new, third-party dependencies (in package.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.

@tdonohue tdonohue added this to the 7.3 milestone Apr 27, 2022
@artlowel artlowel self-requested a review April 28, 2022 12:32
@tdonohue tdonohue self-requested a review April 28, 2022 14:51
Copy link
Copy Markdown
Member

@artlowel artlowel left a comment

Choose a reason for hiding this comment

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

Thanks @atarix83! It looks unobtrusive, and it works.

The performance, while it's noticeably worse than the current main branch on higher page sizes, it is also better than I expected (at least on my machine), so I think this way of working may be feasible. I added some inline comments to the method that does the size calculations, because I think there are a few easy optimizations that can still be made

That said, @tdonohue it would be good if a few other people gave it a test with performance in mind as well (e.g. using high page sizes, grid view, resizing the window, etc), since I have an atypical setup

Comment thread src/app/shared/truncatable/truncatable-part/truncatable-part.component.ts Outdated
Comment thread src/app/shared/truncatable/truncatable-part/truncatable-part.component.ts Outdated
@atarix83
Copy link
Copy Markdown
Contributor Author

@artlowel

we'll work to your feedback in the next days

atarix83 and others added 3 commits May 25, 2022 09:30
# Conflicts:
#	src/app/admin/admin-search-page/admin-search-results/admin-search-result-grid-element/item-search-result/item-admin-search-result-grid-element.component.spec.ts
@atarix83
Copy link
Copy Markdown
Contributor Author

@artlowel

following your feedback we tried to improve performance issue, could you check again? thanks

@atarix83 atarix83 requested a review from artlowel May 26, 2022 17:03
Copy link
Copy Markdown
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.

👍 Thanks @atarix83 ! The code looks good. I tested it and it works well... I noticed that the "Show more" also works for long lists of authors (and longer titles when using the grid view)

I also paid close attention to performance, even using 100 items per page. I'm not seeing any noticeable change in performance with this usability improvement...if there is any change, it's very minor. The only very minor thing I noticed is that on grid view, sometimes the "Show more" link takes a brief moment to appear (less than a second) after the page loads. Overall, this all looks good to me! I feel it's good enough and we can always improve it later as needed. Thanks!

@atarix83
Copy link
Copy Markdown
Contributor Author

@tdonohue

we have detected an issue where truncatable part that had the same number of line as the minimun number lines to show had the faded effect without the show more button. Now it's fixed, could you check again?

Before the fix
Schermata da 2022-05-26 17-18-53

after the fix
Schermata da 2022-05-31 09-10-17

@atarix83 atarix83 requested a review from tdonohue May 31, 2022 07:14
Copy link
Copy Markdown
Member

@artlowel artlowel left a comment

Choose a reason for hiding this comment

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

Thanks for the changes. The performance looks good!

I see a new issue on the grid view now though, perhaps related to the fix to @tdonohue's last feedback?

image

A fade-out is rendered on top right side of thumbnail

@atarix83
Copy link
Copy Markdown
Contributor Author

atarix83 commented Jun 7, 2022

@artlowel

issue should be fixed now, could you check again ?

@atarix83 atarix83 requested a review from artlowel June 7, 2022 15:25
Copy link
Copy Markdown
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.

👍 Thanks @atarix83 ! I re-tested this today, and I'm no longer able to reproduce any of the prior issues that I saw (e.g. the minor issue where "show more" appeared on grid view slightly after page load). I also can no longer reproduce the issue that @artlowel saw (where the fade out was rendered at the top of the thumbnail.)

All in all, this looks good to me, thanks!

@artlowel
Copy link
Copy Markdown
Member

artlowel commented Jun 9, 2022

Thanks @atarix83! The issues are fixed

One more thing I noticed though: is it possible to leave out those "show more" links in the admin sidebar dso search popups? They almost double the height of each result, and you can't actually click them there

Screen Shot 2022-06-09 at 11 06 54

Perhaps leave them out any time the linkType of an object collection element is none because that means you won't be able to click the show more links anyway

@atarix83
Copy link
Copy Markdown
Contributor Author

@artlowel

problem should be resolved

Copy link
Copy Markdown
Member

@artlowel artlowel left a comment

Choose a reason for hiding this comment

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

Thanks @atarix83!

@tdonohue
Copy link
Copy Markdown
Member

Merging as this is at +2. Thanks @atarix83 !

@tdonohue tdonohue merged commit 9db9441 into DSpace:main Jun 13, 2022
@abollini abollini deleted the DSC-516-Style-New branch June 14, 2022 16:01
4science-it pushed a commit to 4Science/dspace-angular that referenced this pull request May 14, 2024
…est DSpace#1619)

Task/dspace cris 2023 02 x/DSC-1626 link support

Approved-by: Stefano Maffei
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.

Improve usability of TruncatableComponet

3 participants