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

[full-ci] Implement long breadcrumb strategy #8984

Merged
merged 52 commits into from
May 24, 2023
Merged

Conversation

lookacat
Copy link
Contributor

@lookacat lookacat commented May 4, 2023

Description

Breadcrumb should not line-break. This is a new strategy to deal with long breadcrumbs and long folder names.
See #6731

Maybe this helps
breadcrumb drawio

Screenshots

Before (old):
old

After (new):
after

Related Issue

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

Open tasks:

  • Fix ... button
  • vertical alignment of breadcrumb items is off. The last breadcrumb item has a lower baseline as all the other elements. More precisely, the breadcrumb items (except the last one) have slightly moved up. You can see that when you compare to the old state and watch the distance between text baseline and the chevron right icon.
  • The breadcrumbs are truncated far too early. See video. There is a lot of space left but the breadcrumbs already get shrinked down to a minimum. That feels wrong.

@update-docs
Copy link

update-docs bot commented May 4, 2023

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

@lookacat lookacat marked this pull request as ready for review May 9, 2023 13:00
@ownclouders
Copy link
Contributor

ownclouders commented May 9, 2023

Results for e2e-tests oCIS https://drone.owncloud.com/owncloud/web/35587/12/1

💥 To see the trace, please open the link in the console ...

npx playwright show-trace https://cache.owncloud.com/public/owncloud/web/35587/tracing/deny-and-grant-access-alice-2023-5-10-01-54-52.zip

@lookacat lookacat changed the title Implement long breadcrumb strategy [full-ci] Implement long breadcrumb strategy May 10, 2023
@ownclouders
Copy link
Contributor

Results for acceptance oC10 https://drone.owncloud.com/owncloud/web/35580/20/1

💥 The acceptance tests failed on retry. Please find the screenshots inside ...

webUIPreview-mediaPreview_feature-L143.png

webUIPreview-mediaPreview_feature-L143.png

@JammingBen
Copy link
Contributor

Haven't looked into the code yet, but what I've noticed testing this:

  • Breadcrumb items get "cut" when exceeding a specific length. See for example "a folder with a very long name" -> "a folder with a":
image
  • Clicking on the ... doesn't render a drop menu with the omitted items, but just goes one level up. Is that intentional?
  • The mobile navigation looks broken:
image

@lookacat
Copy link
Contributor Author

lookacat commented May 10, 2023

Clicking on the ... doesn't render a drop menu with the omitted items, but just goes one level up. Is that intentional?

yes, it should navigate to the previous hidden folder, so when you e.g. only have one folder in the breadcrumb you can navigate one up.

Will look into mobile, and the long folder name

Copy link
Contributor

@JammingBen JammingBen left a comment

Choose a reason for hiding this comment

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

You are obviously much deeper into the topic, but one thing got me wondering: Do you really need to pass calculateBreadcrumbMaxWidth as a method to OcBreadcrumb? Couldn't you just pass a number property (e.g. availableWidth) to the component? You'd simply need to watch this value and re-evaluate the breadcrumb on change. Also, it would make the resize observer obsolete (we already have one in Appbar.vue).

@lookacat
Copy link
Contributor Author

i think that should be possible

@lookacat lookacat force-pushed the long-breadcrumb-strategy branch from 6da59d2 to b2c8dbe Compare May 17, 2023 12:26
@lookacat lookacat requested a review from JammingBen May 17, 2023 12:29
Copy link
Member

@kulmann kulmann left a comment

Choose a reason for hiding this comment

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

Didn't look into the code, yet. Just tried it out. Two things that caught my eye:

  1. vertical alignment of breadcrumb items is off. The last breadcrumb item has a lower baseline as all the other elements. More precisely, the breadcrumb items (except the last one) have slightly moved up. You can see that when you compare to the old state and watch the distance between text baseline and the chevron right icon. See two screenshots:

before

Screenshot 2023-05-17 at 14 54 48

after

Screenshot 2023-05-17 at 14 54 54
  1. The breadcrumbs are truncated far too early. See video. There is a lot of space left but the breadcrumbs already get shrinked down to a minimum. That feels wrong.
Screen.Recording.2023-05-17.at.14.57.13.mov

@lookacat lookacat requested review from kulmann and JammingBen May 19, 2023 13:10
@lookacat lookacat requested review from AlexAndBear and kulmann May 23, 2023 08:38
@lookacat lookacat force-pushed the long-breadcrumb-strategy branch from 6656c00 to be25c9d Compare May 23, 2023 08:42
@lookacat lookacat force-pushed the long-breadcrumb-strategy branch from be25c9d to 80a5a65 Compare May 23, 2023 09:34
@lookacat lookacat requested a review from AlexAndBear May 23, 2023 14:52
Copy link
Member

@kulmann kulmann left a comment

Choose a reason for hiding this comment

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

Probably looks like a wall of text (sorry for that) but all of these comments are small stuff and should be easy to fix... Great work, love it! UX feels really good.

hiddenItems.value = []

nextTick(() => {
reduceBreadcrumb(2)
Copy link
Member

Choose a reason for hiding this comment

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

Please introduce a param for the offset. Don't forget to use it when inserting the ... element to the correct position. Use 2 as default for the offset param. But then set 3 for breadcrumbs in project spaces. That way you get Spaces > SpaceName > ... > trölölö for project spaces. Otherwise the space name will always be cut off.

Copy link
Contributor Author

@lookacat lookacat May 24, 2023

Choose a reason for hiding this comment

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

i like the idea very much and i implemented it BUT, as soon as i navigate into a space im no longer in a project space but in a generic space, the same as personal

Copy link
Member

Choose a reason for hiding this comment

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

But you do have a space object via prop in the generic space view. And on that space object you can use the type guard isProjectSpaceResource(props.space) to identify if it's a project space or not. I think you also need the same with isShareSpaceResource(props.space) because a share breadcrumb also has an overview item and a share root item in the breadcrumbs (Shared with me > some share name > ... > subfolder).

@lookacat lookacat requested a review from kulmann May 24, 2023 08:05
@AlexAndBear AlexAndBear requested a review from kulmann May 24, 2023 13:31
@kulmann
Copy link
Member

kulmann commented May 24, 2023

Screenshot 2023-05-24 at 16 01 08

Copy link
Member

@kulmann kulmann left a comment

Choose a reason for hiding this comment

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

🥳

@sonarcloud
Copy link

sonarcloud bot commented May 24, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

87.8% 87.8% Coverage
0.0% 0.0% Duplication

@kulmann kulmann merged commit a9898e8 into master May 24, 2023
@delete-merged-branch delete-merged-branch bot deleted the long-breadcrumb-strategy branch May 24, 2023 20:34
@tbsbdr tbsbdr mentioned this pull request Jun 30, 2023
37 tasks
@tbsbdr tbsbdr added this to the CERN Web Merge milestone Jul 10, 2023
@micbar micbar mentioned this pull request Jul 24, 2023
68 tasks
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.

Long breadcrumb: truncate and add drop menu
6 participants