feat(ui5-breadcrumbs): Initial implementation #3489

kineticjs · 2021-07-05T05:04:38Z

packages/main/src/Label.js

nnaydenow · 2021-07-05T10:18:42Z

packages/main/src/Link.js

@@ -241,6 +254,9 @@ class Link extends UI5Element {
 	}

 	get tabIndex() {
+		if (this._tabIndex.length) {


it will be undefined, -1 (valid and will be set) or 0 (valid and will be set) so we don't need to check length here

nnaydenow · 2021-07-05T10:24:37Z

packages/main/src/themes/Label.css

@@ -77,3 +77,9 @@ bdi {
 	margin-right: .125rem;
 	margin-left: 0;
 }
+
+.ui5-label-root:focus {


:host([focused]) .ui5-label-root since the focused attribute is updated on focusin/focusout

the label does not have a "focused" property... do you think we should add one, for consistency with other similar cases?

btw I checked that some components e.g. Carousel also do not define a "focused" property and use the :focus instead

nnaydenow · 2021-07-05T10:25:11Z

packages/main/src/themes/Label.css

+.ui5-label-root:focus {
+	outline-offset: -1px;
+	outline: 1px dotted var(--sapContent_FocusColor);
+	text-decoration: underline;


should be set to ui5-label-text-wrapper otherwise it doesn't affect the text. Also I think we should skip this or to add shadow part to the the text wrapper and to style it from styles of breadcrumbs since it is ui5-breadcrumbs specific @ilhan007

the rule for "text-decoration" was added by me by mistake, I removed it as it is not needed at all

so I just tried to move this css to Breadcrumbs.css, but I do not seem to be allowed to target ui5-label-root from there

I need to target ui5-label-root as this is the element that has the tabIndex attribute (the inner ui5-label-text-wrapper is not focusable, sorry for overlooking the confusing "text-decoration" rule)

nnaydenow · 2021-07-05T10:37:34Z

packages/main/src/BreadcrumbsPopover.hbs

@@ -0,0 +1,29 @@
+<ui5-responsive-popover
+	no-arrow


kineticjs · 2021-07-07T07:24:25Z

packages/main/src/types/BreadcrumbsDesign.js

+ * @lends sap.ui.webcomponents.main.types.BreadcrumbsDesign.prototype
+ * @public
+ */
+const BreadcrumbsTypes = {


btw here the 2 types are defined in accordance with the UX spec at:
https://ux.wdf.sap.corp/fiori-design-web/breadcrumb/#types

dobrinyonkov

Overall, it works and looks great. Just some minor comments about the names of the aria properties, CSS selectors and JSDocs.
Also I don't see a playground sample. Is it intentionally posponed to be added later?

packages/main/src/BreadcrumbsItem.js

packages/main/src/themes/Breadcrumbs.css

packages/main/test/specs/Breadcrumbs.spec.js

packages/main/src/types/BreadcrumbsDesign.js

Fixes SAP#3166

The role presentation on the icon SVG is ignored in some cases: *The element is focusable, e.g. it is natively focusable like an HTML link or input, or it has a tabindex attribute. *The element has any of the twenty-one global ARIA states and properties, e.g., aria-label. With this change we introduce ariaHidden property on ui5-icon to be able to completly hide it from accessibility tree mapping. Also, change the default value of effectiveAccessibleName to undefined as a value of "" will still set the aria-label attribute and the presentation role will be ignroed. Fixes SAP#3433

…mponents into breadcrumbs3

dobrinyonkov · 2021-07-22T04:19:34Z

packages/main/src/Link.js

+		 * @private
+		 * @since 1.0.0-rc.15
+		 */
+		role: {


The text "More button menu" is read out twice upon focusing the ui5-link with overflowed items.
This is because of this attribute being named role. The screen reader reads the role and the label twice. Once from the custom element and once from the internal shadow dom element. If we rename this property to accRole the announcment is correct.

Maybe accessibleRole, to be consistent with the List and Panel components, where this is how it is named.

Renamed to "accessibleRole"

ilhan007 · 2021-07-22T11:34:04Z

packages/main/src/Breadcrumbs.js

+
+		/**
+		 * Determines the visual style of the separator between the breadcrumb items.
+		 *


List the options in the description in this way:

* <br><br> * Available options are: * <ul> * <li><code>TopStart</code></li> * <li><code>BottomCenter</code></li> * <li><code>BottomEnd</code></li> * </ul>

ilhan007 · 2021-07-22T11:35:16Z

packages/main/src/Breadcrumbs.js

+ * @class
+ *
+ * <h3 class="comment-api-title">Overview</h3>
+ * Enables users to navigate between items by providing a list of links to previous steps in the user's navigation path. The last three steps can be accessed as links directly, while the remaining links prior to them are available in a drop-down menu.


If you have a KM, perhaps you can involve him/her to review or add more information if necessary.

ilhan007 · 2021-07-22T11:36:31Z

packages/main/src/Breadcrumbs.js

+		 * @param {HTMLElement} link The clicked link.
+		 * @public
+		 */
+		"link-click": {


item-click might be also a candidate, as the child component is BreadcrumbsItem and the Breadcrumbs slot is referred as "items" (although default), maybe you can consider that.

yes, I pondered earlier about the event name!
The name "item-click" is consistent with the slot name, but I named it "link-click" because currently no event is fired if the clicked item is the one that stands for the "current-location" label. For now I see two options:

fire the event even when the "current-location" label is pressed - but if we want to keep the label as non-interactive, then I discard this option
OR

rename the event from "link-click" to "item-click" nevertheless, for consistence with the slot name

What do you think?
Thnx!

Eventually refactored to "item-click" that fires on click on any item, as it makes most sense

ilhan007 · 2021-07-22T11:39:35Z

packages/main/src/BreadcrumbsItem.js

+*/
+const metadata = {
+	tag: "ui5-breadcrumbs-item",
+	managedSlots: true,


I think you don't need "managedSlots: true," here, probably copied.

I added "managedSlots: true" to be able to react (recalculate the number of overflowing links) whenever the text of the link changes.

Do you have a better idea in mind?

Btw earlier I used a ResizeHandler on a content wrapper, but seemed like the less efficient solution, especially to monitor changes in the size of links that are currently in the overflow.

kineticjs · 2021-07-28T15:11:53Z

Hi @gmkv,

I agree to not set focus on the label itself but on a wrapper element. Done.

CLAassistant · 2021-07-31T07:44:29Z

All committers have signed the CLA.

kineticjs · 2021-08-02T07:55:02Z

all requested changes of @xtatica are integrated

dobrinyonkov · 2021-08-02T10:04:54Z

packages/main/src/BreadcrumbsItem.js

+ * <h3 class="comment-api-title">Overview</h3>
+ *
+ * The <code>ui5-breadcrumbs-item</code> component defines the content of an item in <code>ui5-breadcumbs</code>.
+ *


We could add a <h3>Keyboard Handling</h3> section somewhere here with supported key shortcuts . Similar to other components, see ui5-select for example :)

ilhan007

The code looks good to me, @SAP/ui5-webcomponents-topic-p it's up to you to finish the review.

outdated - Documentation Review addressed

kineticjs · 2021-08-03T07:03:24Z

Thanks to all involved for the code review of this big change...! Waiting for @nnaydenow for final vote.

nnaydenow

@kineticjs looks good to me. @ilhan007 could you please confirm which version we should use in JSDoc? I'm not sure if the rc-15 is already released and in which version we will release that component.

nnaydenow · 2021-08-03T09:51:17Z

packages/main/src/Link.js

+		 * @type String
+		 * @defaultvalue undefined
+		 * @private
+		 * @since 1.0.0-rc.16


I'm not sure about this version. @ilhan007 could you check please?

ilhan007 · 2021-08-03T10:17:58Z

Let's keep it RC.16, we can update that any time if needed. We are about to release RC.15 including IllustratedMessage and BarcodeScannerDialog (thanks for these two as well) and we plan to leave the Breadcrumbs for the next stable version.

Fixes #3166

kineticjs force-pushed the breadcrumbs3 branch from 5500524 to 3f1d4da Compare July 5, 2021 05:13

kineticjs requested review from nnaydenow, dobrinyonkov and ilhan007 July 5, 2021 05:49

nnaydenow reviewed Jul 5, 2021

View reviewed changes

kineticjs force-pushed the breadcrumbs3 branch 3 times, most recently from 86306b8 to 4c8772b Compare July 7, 2021 07:20

kineticjs commented Jul 7, 2021

View reviewed changes

dobrinyonkov requested changes Jul 12, 2021

View reviewed changes

kineticjs force-pushed the breadcrumbs3 branch 7 times, most recently from c2152b2 to 57243a0 Compare July 14, 2021 19:00

kineticjs and others added 2 commits July 20, 2021 12:37

feat(ui5-breadcrumbs): Initial implementation

d229706

Fixes SAP#3166

kineticjs force-pushed the breadcrumbs3 branch from 99ebf23 to 6e1306c Compare July 20, 2021 09:38

dobrinyonkov and others added 2 commits July 20, 2021 14:37

Merge branch 'breadcrumbs3' of https://github.com/kineticjs/ui5-webco…

39bbdb9

…mponents into breadcrumbs3

kineticjs requested review from dobrinyonkov and nnaydenow July 20, 2021 12:19

dobrinyonkov requested changes Jul 22, 2021

View reviewed changes

ilhan007 added the New Component label Jul 22, 2021

ilhan007 reviewed Jul 22, 2021

View reviewed changes

Merge remote-tracking branch 'origin/master' into breadcrumbs3

223e742

georgimkv previously approved these changes Jul 29, 2021

View reviewed changes

adopt showAt function for popover

311457e

kineticjs dismissed georgimkv’s stale review via 311457e July 29, 2021 05:56

kineticjs added 3 commits August 1, 2021 22:57

corrected update of size cache for empty links

9025715

improve playground sample

cd6a11f

Merge remote-tracking branch 'origin/master' into breadcrumbs3

18ca3b5

kineticjs requested review from ilhan007, dobrinyonkov and georgimkv August 2, 2021 07:47

dobrinyonkov reviewed Aug 2, 2021

View reviewed changes

kineticjs added 5 commits August 2, 2021 13:05

enhance sample page to demonstrate the separator types

6e32852

improve keyboard handling docu and code

2c663c6

improve keyboard handling docu

abff553

rename overflowLink to dropdownArrowLink

fd1f218

remove redundant parenthesis

42fd0a5

ilhan007 approved these changes Aug 2, 2021

View reviewed changes

kineticjs requested review from dobrinyonkov and removed request for georgimkv August 2, 2021 18:45

dobrinyonkov approved these changes Aug 3, 2021

View reviewed changes

nnaydenow reviewed Aug 3, 2021

View reviewed changes

ilhan007 merged commit 6dbc2a0 into SAP:master Aug 3, 2021

ilhan007 pushed a commit that referenced this pull request Aug 4, 2021

feat(ui5-breadcrumbs): Initial implementation (#3489)

0f03ccf

Fixes #3166

ilhan007 mentioned this pull request Aug 4, 2021

New Component: ui5-barcode-scanner #3187

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ui5-breadcrumbs): Initial implementation #3489

feat(ui5-breadcrumbs): Initial implementation #3489

kineticjs commented Jul 5, 2021

nnaydenow Jul 5, 2021

kineticjs Jul 7, 2021

nnaydenow Jul 5, 2021

kineticjs Jul 7, 2021

nnaydenow Jul 5, 2021 •

edited

Loading

kineticjs Jul 7, 2021

nnaydenow Jul 5, 2021

kineticjs Jul 7, 2021

kineticjs Jul 7, 2021

dobrinyonkov left a comment

dobrinyonkov Jul 22, 2021

ilhan007 Jul 22, 2021

kineticjs Jul 26, 2021

ilhan007 Jul 22, 2021

kineticjs Jul 26, 2021

ilhan007 Jul 22, 2021

ilhan007 Jul 22, 2021

kineticjs Jul 26, 2021

kineticjs Jul 26, 2021

ilhan007 Jul 22, 2021

kineticjs Jul 26, 2021

kineticjs commented Jul 28, 2021

CLAassistant commented Jul 31, 2021 •

edited

Loading

kineticjs commented Aug 2, 2021

dobrinyonkov Aug 2, 2021

ilhan007 left a comment

kineticjs commented Aug 3, 2021

nnaydenow left a comment

nnaydenow Aug 3, 2021

ilhan007 commented Aug 3, 2021

feat(ui5-breadcrumbs): Initial implementation #3489

feat(ui5-breadcrumbs): Initial implementation #3489

Conversation

kineticjs commented Jul 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nnaydenow Jul 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dobrinyonkov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kineticjs commented Jul 28, 2021

CLAassistant commented Jul 31, 2021 • edited Loading

kineticjs commented Aug 2, 2021

Choose a reason for hiding this comment

ilhan007 left a comment

Choose a reason for hiding this comment

kineticjs commented Aug 3, 2021

nnaydenow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilhan007 commented Aug 3, 2021

nnaydenow Jul 5, 2021 •

edited

Loading

CLAassistant commented Jul 31, 2021 •

edited

Loading