Skip to content

Fixes breadcrumb wrapping and overlapping the select all box#4976

Merged
akolson merged 5 commits intolearningequality:unstablefrom
AllanOXDi:truncate-long-breadcrumb-title
May 15, 2025
Merged

Fixes breadcrumb wrapping and overlapping the select all box#4976
akolson merged 5 commits intolearningequality:unstablefrom
AllanOXDi:truncate-long-breadcrumb-title

Conversation

@AllanOXDi
Copy link
Copy Markdown
Member

Summary

This PR fixes the overlapping of the breadcrumbs
Closes #4829

Before

Screen.Recording.2025-03-27.at.21.43.45.mov

After

Screen.Recording.2025-03-27.at.21.42.04.mov

References

#4829

@AllanOXDi AllanOXDi requested review from akolson and bjester March 27, 2025 18:51

.text-truncate{
overflow: hidden;
width:100px;
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 this would not only partially resolve the issue but would also introduce responsiveness concerns. Besides, if we really need to override the width of the component, it makes more sense do so within a wrapper instead, I think (this styling has been defined within the component itself). Better still, we can make the change in the Breadcrumbs component where I suspect the issues resides. It appears to me that the finickiness is due to the way we are handling the overflow calculations, so it will require some digging around to establish what is causing the issue.

handleOverflow() {
const maxWidth = this.$refs.breadcrumbs.$el.offsetWidth;
let totalWidth = 0;
this.breadcrumbStartingIndex = 0;
this.$nextTick(() => {
if (this.$refs.breadcrumb) {
for (var i = this.$refs.breadcrumb.length - 1; i >= 0; --i) {
totalWidth += this.$refs.breadcrumb[i].$el.offsetWidth + 40;
// Bounds exceeded, go back to previous index
if (totalWidth >= maxWidth - 48) {
this.breadcrumbStartingIndex = Math.min(this.items.length - 1, i + 1);
break;
}
}
}
});

Copy link
Copy Markdown
Member

@akolson akolson left a comment

Choose a reason for hiding this comment

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

Hi @AllanOXDi. Left a few comments

@AllanOXDi AllanOXDi changed the base branch from unstable to hotfixes April 14, 2025 14:27
@AllanOXDi AllanOXDi changed the base branch from hotfixes to unstable April 14, 2025 14:28
@AllanOXDi AllanOXDi force-pushed the truncate-long-breadcrumb-title branch from ba1ec40 to 34cd432 Compare April 15, 2025 11:01
@AllanOXDi AllanOXDi changed the base branch from unstable to hotfixes April 15, 2025 11:02
@AllanOXDi AllanOXDi force-pushed the truncate-long-breadcrumb-title branch from 34cd432 to ba1ec40 Compare April 15, 2025 11:06
@AllanOXDi AllanOXDi changed the base branch from hotfixes to unstable April 15, 2025 11:06
@AllanOXDi AllanOXDi force-pushed the truncate-long-breadcrumb-title branch from ba1ec40 to 4edb95e Compare April 17, 2025 14:16
@AllanOXDi AllanOXDi changed the base branch from unstable to hotfixes April 17, 2025 14:18
@AllanOXDi AllanOXDi changed the base branch from hotfixes to unstable April 17, 2025 14:18
@AllanOXDi AllanOXDi force-pushed the truncate-long-breadcrumb-title branch from 4edb95e to 2b9ac06 Compare April 17, 2025 14:26
@AllanOXDi AllanOXDi changed the base branch from unstable to hotfixes April 17, 2025 14:27
Comment thread requirements.txt
Copy link
Copy Markdown
Member

@akolson akolson Apr 22, 2025

Choose a reason for hiding this comment

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

Not too sure we want to commit this deleted file?

Comment thread yarn.lock Outdated
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.

this.$nextTick(() => {
if (this.$refs.breadcrumb) {
for (var i = this.$refs.breadcrumb.length - 1; i >= 0; --i) {
totalWidth += this.$refs.breadcrumb[i].$el.offsetWidth + 40;
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.

Unfortunately, this still doesn't fix the issue. If anything, it would be a partial fix--in some screen sizes it works on others it doesn't

@AllanOXDi AllanOXDi force-pushed the truncate-long-breadcrumb-title branch from e77f2c3 to f83d1b7 Compare May 7, 2025 15:27
@akolson
Copy link
Copy Markdown
Member

akolson commented May 9, 2025

Hi @AllanOXDi Could you please retarget this to unstable? (You might have to rebase your branch on unstable)

@AllanOXDi AllanOXDi force-pushed the truncate-long-breadcrumb-title branch from f83d1b7 to 29a0ca2 Compare May 9, 2025 13:15
@AllanOXDi AllanOXDi changed the base branch from hotfixes to unstable May 9, 2025 13:15
Comment thread yarn.lock Outdated
Comment thread requirements.txt Outdated
@AllanOXDi AllanOXDi force-pushed the truncate-long-breadcrumb-title branch from 44fec86 to 410c928 Compare May 9, 2025 15:30
Copy link
Copy Markdown
Member

@akolson akolson left a comment

Choose a reason for hiding this comment

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

Changes look correct to me. I have also done some manual QA (for some time now) on the fix but haven't been able to replicate the issue again. (More manual QA required). Thanks @AllanOXDi for your persistence on this issue!

@akolson akolson merged commit aad86b9 into learningequality:unstable May 15, 2025
13 checks passed
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.

Investigate breadcrumb wrapping and overlapping the select all box

3 participants