Skip to content

Use full community, collection and item names in breadcrumb navigation#1957

Merged
tdonohue merged 4 commits intoDSpace:mainfrom
jtimal:1794
Dec 15, 2022
Merged

Use full community, collection and item names in breadcrumb navigation#1957
tdonohue merged 4 commits intoDSpace:mainfrom
jtimal:1794

Conversation

@jtimal
Copy link
Copy Markdown
Contributor

@jtimal jtimal commented Nov 7, 2022

Description

Small PR based in Angular Issue

Expected result:
image

modify the breadcumbs html file to show the full name on mouseover
@tdonohue tdonohue added bug usability 1 APPROVAL pull request only requires a single approval to merge labels Nov 7, 2022
@tdonohue tdonohue added this to the 7.5 milestone Nov 7, 2022
@tdonohue
Copy link
Copy Markdown
Member

tdonohue commented Dec 7, 2022

@jtimal : This looks to be failing a single spec/test... the should render the breadcrumbs test here in breadcrumbs.component.spec.ts: https://github.com/DSpace/dspace-angular/blob/main/src/app/breadcrumbs/breadcrumbs.component.spec.ts#L75

Could you look into this? It seems like it's directly related to the small changes you've made in this PR...and hopefully it'll be an easy test to fix.

@jtimal
Copy link
Copy Markdown
Contributor Author

jtimal commented Dec 8, 2022

@jtimal : This looks to be failing a single spec/test... the should render the breadcrumbs test here in breadcrumbs.component.spec.ts: https://github.com/DSpace/dspace-angular/blob/main/src/app/breadcrumbs/breadcrumbs.component.spec.ts#L75

Could you look into this? It seems like it's directly related to the small changes you've made in this PR...and hopefully it'll be an easy test to fix.

Thank you Tim,

I have already made a correction and it seems to pass the tests.

Thanks!

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.

@jtimal: Thanks for this fix! I tested it today and it works well overall. I'm nearly a +1, but I'd recommend making a few minor changes to this PR before we merge this. See my inline comments below.

Comment thread src/app/breadcrumbs/breadcrumbs.component.html Outdated
expectBreadcrumb(breadcrumbs[0], 'home.breadcrumbs', '/');
expectBreadcrumb(breadcrumbs[1], 'bc 1', '/example.com');
expectBreadcrumb(breadcrumbs[2].query(By.css('.text-truncate')), 'bc 2', null);
expectBreadcrumb(breadcrumbs[2].query(By.css('.text-truncate')), ' bc 2', null);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think you can undo this change once you make the changes I described above.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hi @tdonohue,

I have finished with the suggestions and uploaded the changes.

Thank you very much for your comments.

Regards!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@jtimal i see this change is still present, can you undo it? thanks

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

hi @atarix83 I have already undone the change

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@jtimal : The problem was that when you undid the change, the alignment in that file got accidentally messed up. I just fixed it by pushing a commit to your branch: 8c8df56

With that fix, the changes to breadcrumbs.component.spec.ts have been fully reverted, so that it no longer appears in the "Files changed" tab at the top of this PR.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@tdonohue ok, thank you very much and an apology for this.

Copy link
Copy Markdown
Contributor

@atarix83 atarix83 left a comment

Choose a reason for hiding this comment

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

thanks @jtimal for this PR

i have only a minor change request.
The tooltip is shown only for active link of the breadcrumb, the final part doesn't have it. I would ad the same tooltip also there, e.g.

Schermata da 2022-12-15 15-08-55

@tdonohue
Copy link
Copy Markdown
Member

tdonohue commented Dec 15, 2022

@atarix83 : I asked @jtimal to remove the tooltip from the final part of the breadcrumbs. It's not necessary to show the tooltip in that final part, as the full title is visible just below the breadcrumbs. In other words, tooltips should only be displayed for titles of parent objects in the breadcrumbs.

This is similar to 6.x behavior, as on the Item page itself, 6.x never displayed the full title in the breadcrumbs. Instead it just displayed "View Item". In 7.x, we choose to display a snippet of the title...but there's no need to show the full title in the breadcrumbs.

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.

👍 Re-tested today. Thanks @jtimal ! This looks good to me now.

In today's meeting, @atarix83 and I briefly discussed this and agreed that the tooltip isn't necessary on the last part of the breadcrumb (the current page) as the title is always displayed in full on the current page.

Therefore, I'm going to go ahead and merge this immediately.

@jtimal
Copy link
Copy Markdown
Contributor Author

jtimal commented Dec 15, 2022

👍 Re-tested today. Thanks @jtimal ! This looks good to me now.

In today's meeting, @atarix83 and I briefly discussed this and agreed that the tooltip isn't necessary on the last part of the breadcrumb (the current page) as the title is always displayed in full on the current page.

Therefore, I'm going to go ahead and merge this immediately.

Thank you very much for all your help !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

1 APPROVAL pull request only requires a single approval to merge bug usability

Projects

No open projects
Status: ✅ Done

Development

Successfully merging this pull request may close these issues.

3 participants