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

fix(breadcrumb): Returning a promise from computeVisibilityMap - FRONT-4314 #3439

Merged
merged 8 commits into from
Jun 24, 2024

Conversation

planctus
Copy link
Contributor

@planctus planctus commented Jun 18, 2024

Basically the calculations were failing at page load and this made the breadcrumb collapsed by default, pretty much always.
A timeout has been introduced to delay the calculations and this made them reliable, in order to be able to use the visibility map for how it was configured the check method is now async so that we can await for the visibility map to be ready.

Copy link

github-actions bot commented Jun 18, 2024

@github-actions github-actions bot temporarily deployed to pull request June 18, 2024 11:14 Inactive
@github-actions github-actions bot temporarily deployed to pull request June 18, 2024 11:54 Inactive
@emeryro
Copy link
Contributor

emeryro commented Jun 20, 2024

On my side, these changes did not really fix the initial display issue of the breadcrumb.
I tested it in Storybook, so it may be the reason why it failed. Using the demo data, and a screen of 900px width for instance, the breadcrumb appears collapsed by default, while there is in fact enough space to display it all
image
image

Also I'm a little scared that the timeout would add a page jump at load, as the content would be pushed down then up when the breadcrumb collapses

@github-actions github-actions bot temporarily deployed to pull request June 20, 2024 08:56 Inactive
@planctus
Copy link
Contributor Author

planctus commented Jun 20, 2024

So..the calculations are now consistent enough, but in the js we are doing this: if (allItemsWidth * 1.2 <= wrapperWidth) to limit the risk of expanding the breadcrumb in edge situations. This is why even with enough space you got it collapsed, i reduced this to 10% now, which seems pretty much ok, but obviously like this we have less margin for errors.

Furthermore i noticed somethign tricky, we do have a complex way of calling the autoInit() in storybook, i do remember alexis struggling in order to get things like RTL to work which lead to the current code, but the issue is that sometimes the init is not called at all, it happens for me when pressing ENTER in the url bar, to re-load the page, in those cases the js doesn't kick in at all and the breadcrumb is potentially displayed in multiple lines.
I will create a ticket to investigate this as any change here might have unexpected consequences.

To limit the issues with "jumping content", i have added flex-wrap: nowrap; when the component has been initialized, we always show the breadcrumb on one line, basing on the code we have, so this way the "height" of the element will be fixed and i set the ellipsis not to be visible by default, but still there might be an horizontal jumping, less critical and unavoidable unless we "hide" the breadcrumb and we show it when the calculations are done, which is something we haven't done until now for any of the components we have.

@github-actions github-actions bot temporarily deployed to pull request June 20, 2024 09:08 Inactive
@emeryro
Copy link
Contributor

emeryro commented Jun 20, 2024

It's already much better.
I would suggest to use a fixed value for the "safety net" used in width comparison. That's what we do in the menu for instance
https://github.com/ec-europa/europa-component-library/blob/v4-dev/src/implementations/vanilla/components/menu/menu.js#L640
That would avoid having potentially more issues as the screen goes larger

@planctus
Copy link
Contributor Author

mmh..i'm confused, it seems the other way around, the risk we try to limit is to expand the menu when there might not be enough space and what we do in the menu is much less "safe" than what we do here, in the menu we add 16px to the container width, here we add 10% to the items width, i wouldn't push this more than we did because there are already cases where we get the breadcrumb reaching the edge of the container.

@github-actions github-actions bot temporarily deployed to pull request June 21, 2024 07:58 Inactive
@emeryro emeryro merged commit f977b4f into v4-dev Jun 24, 2024
7 checks passed
@emeryro emeryro deleted the FRONT-4314-Breadcrumb branch June 24, 2024 06:59
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.

2 participants