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

feat(ui5-breadcrumbs): Initial implementation #3435

Closed
wants to merge 1 commit into from

Conversation

kineticjs
Copy link
Contributor

@kineticjs kineticjs commented Jun 16, 2021

Fixes #3166

Two implementations are available:

  1. ui5-breadcrumbs with "current-location" property e.g.:
    <ui5-breadcrumbs current-location="Laptop"> <ui5-link>Products</ui5-link> <ui5-link>Suppliers</ui5-link> <ui5-link>Titanium</ui5-link> </ui5-breadcrumbs>
    Code: https://github.com/kineticjs/ui5-webcomponents/tree/breadcrumbs

  2. ui5-breadcrumbs with links only e.g.:
    <ui5-breadcrumbs> <ui5-link>Products</ui5-link> <ui5-link>Suppliers</ui5-link> <ui5-link>Titanium</ui5-link> <ui5-link>Current</ui5-link> </ui5-breadcrumbs>
    Code: https://github.com/kineticjs/ui5-webcomponents/tree/breadcrumbs1

THIS PR IS FOR THE 2nd INPLEMENTATION (links only), because its API seemed to be preferred
However, both versions are proposed, esp. as the 1st seem better to me from performance perspective)

Note:
The 2nd implementation (links only) seemed to require some extra performance cost.
This is because the last slotted link is used to indicate the current location
=> the text of the last slotted link is rendered as ui5-label
=> I had to ensure we are notified for changes in the text of the last link
=> I had to additionally declare:
-- ui5-link declared to have managedSlots = true
-- ui5-breadcrumbs declared to invalidateOnChildChange: {
properties: false,
slots: true,
}
No such need detected for implementation 1 (with "current-location" property) as it only listens for resize of its content.

Feel free to propose improvements. I will complete the version (1 or 2) that we eventually choose.

@CLAassistant
Copy link

CLAassistant commented Jun 16, 2021

CLA assistant check
All committers have signed the CLA.

@ilhan007 ilhan007 closed this Jun 18, 2021
@ilhan007 ilhan007 reopened this Jun 18, 2021
@kineticjs kineticjs force-pushed the breadcrumbs1 branch 7 times, most recently from ba5ed3e to 8c4829d Compare June 23, 2021 08:24
@kineticjs kineticjs requested review from nnaydenow and ilhan007 June 23, 2021 08:48
@kineticjs
Copy link
Contributor Author

I'm still investigating an issue will failing tests (popovers cannot be opened on the remote server even though no issue in local executions)
but open the PR for review in order to collect early feedback regarding which implementation version (of the two described above) you find better.

@kineticjs kineticjs force-pushed the breadcrumbs1 branch 2 times, most recently from d41c580 to 48665a2 Compare June 24, 2021 07:59
@kineticjs
Copy link
Contributor Author

The the issue with failing tests is resolved

@kineticjs kineticjs marked this pull request as ready for review June 24, 2021 14:14
@kineticjs kineticjs force-pushed the breadcrumbs1 branch 4 times, most recently from 269952b to a1a99b2 Compare June 29, 2021 06:05
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.

New Component: ui5-breadcrumbs
3 participants