updated lib/KListWithOverflow.vue and lib/KBreadcrumbs.vue #857

shruti862 · 2024-12-08T13:07:30Z

Description

Updated lib/KListWithOverflow.vue and lib/KBreadcrumbs.vue files

Issue addressed

Addresses issue #693

Addresses #PR# HERE

Before/after screenshots

Changelog

Description: Summary of change(s)
Products impact: Choose from - none (for internal updates) / bugfix / new API / updated API / removed API. If it's 'none', use "-" for all items below to indicate they are not relevant.
Addresses: Link(s) to GH issue(s) addressed. Include KDS links as well as links to related issues in a consumer product repository too.
Components: Affected public KDS component. Do not include internal sub-components or documentation components.
Breaking: Will this change break something in a consumer? Choose from: yes / no
Impacts a11y: Does this change improve a11y or adds new features that can be used to improve it? Choose from: yes / no
Guidance: Why and how to introduce this update to a consumer? Required for breaking changes, appreciated for changes with a11y impact, and welcomed for non-breaking changes when relevant.

Steps to test

Step 1
Step 2
...

(optional) Implementation notes

At a high level, how did you implement this?

Does this introduce any tech-debt items?

Testing checklist

Contributor has fully tested the PR manually
If there are any front-end changes, before/after screenshots are included
Critical and brittle code paths are covered by unit tests
The change is described in the changelog section above

Reviewer guidance

Is the code clean and well-commented?
Are there tests for this change?
Are all UI components LTR and RTL compliant (if applicable)?
Add other things to check for here

Comments

MisRob · 2024-12-09T04:41:52Z

Converting to draft #693 (comment)

MisRob · 2024-12-09T04:43:34Z

@shruti862 Also referencing @AlexVelezLl's comments that touches on some areas you mentioned, perhaps it helps #693 (comment)

MisRob · 2024-12-09T08:55:17Z

@AlexVelezLl Hi Alex, @shruti862 continues this work and already followed some of your guidance. @shruti862 experienced some blockers and I asked to elaborate in this PR on some specific places. Would you please be available for giving a direction?

cd730f1

shruti862 · 2024-12-09T08:59:36Z

1.) Unable to get separators '>' between the items of list
2.) In the dropdown menu ,only the overflowed text is visible, link is not being applied.

AlexVelezLl · 2024-12-10T14:06:21Z

Thanks @shruti862! I will take a look later today :)

AlexVelezLl

Thank you @shruti862!! This is a great start! I have left some instructions to solve the issues you were experiencing, along with some other things I noticed. Thank you!!

AlexVelezLl · 2024-12-11T13:04:20Z

lib/KListWithOverflow.vue

@@ -274,7 +284,15 @@
  }

  .more-button-wrapper {
-    visibility: hidden;
+    visibility: visible;
+    order: 0; /* Ensure it remains in its original position for keyboard navigation */


We cant have this solution for the position of the button, because Keyboard navigation would not be the same. We need to keep the current behaviour in KBreadcrumbs that if the more button appears at the beginning, the keyboard navigation should focus it first, before the rest items.

AlexVelezLl · 2024-12-11T13:06:42Z

lib/KListWithOverflow.vue

@@ -274,7 +284,15 @@
  }

  .more-button-wrapper {
-    visibility: hidden;
+    visibility: visible;


Hmm interested to know why is this change needed? The more button starts as hidden because we measure its width first, before doing the next computations, so to avoid it being visible for that first render we initialize its visibility as hidden.

AlexVelezLl · 2024-12-11T13:14:37Z

lib/KListWithOverflow.vue

@@ -129,6 +137,8 @@
       */
      setOverflowItems() {
        const { list, listWrapper, moreButtonWrapper } = this.$refs;
+
+        // Exit early if necessary refs are not available


Lets remove these comments, as the code itself is descriptive

AlexVelezLl · 2024-12-11T13:14:58Z

lib/KListWithOverflow.vue

          const itemWidth = itemsSizes[i].width;
-
-          // If the item dont fit in the available space or if we have already


Lets get back these comments that describes a behaviour a little bit more complex

lib/KListWithOverflow.vue

AlexVelezLl · 2024-12-11T14:19:30Z

lib/KBreadcrumbs.vue

+        appearance="raised-button"
+      >
+        <template #menu>
+          <!-- issue :: dropdown menu shows only text of the overflowItems but not the link , I don't know whether i am doing the right way to reference link or not -->


For this you can add these lines in KDropdownMenu:

https://github.com/shruti862/kolibri-design-system/blob/cd730f1821ceef5becb428b01d392dfdf1ae546c/lib/KDropdownMenu.vue#L20-L25

<UiMenu ref="menu" :options="options" :hasIcons="hasIcons" @select="handleSelection" > <template #option="{ option }"> <slot name="option" :option="option"/> </template> </UiMenu>

And then you will be able to customize these dropdown options using this option slot in KDropdownMenu. There we can handle the link behaviour

AlexVelezLl · 2024-12-11T14:21:12Z

lib/KBreadcrumbs.vue

        }
+        // Add separators between items
+        return crumbs.flatMap((item, index) =>
+          index > 0 ? [{ type: 'separator' }, item] : [item]


The reason the dividers are not being visible is because we are passing this as type: "separator" while the correct name is type: "divider"

AlexVelezLl · 2024-12-11T14:21:52Z

lib/KBreadcrumbs.vue

+<style scoped>
+  .breadcrumbs-divider {
+    margin: 0 10px;
+    color: black; /* Adjust color for the ">" separator */


Lets standarize the name, and lets call it always "divider" instead of "separator"

Also, lets not use burned colors. Instead we can use our color tokens, we cant reference these in css, so we will need to set this color in the vue template

AlexVelezLl · 2024-12-11T14:22:41Z

lib/KBreadcrumbs.vue

+          <!-- issue :: dropdown menu shows only text of the overflowItems but not the link , I don't know whether i am doing the right way to reference link or not -->
+          <KDropdownMenu
+            :options="overflowItems
+              .filter(item => item.type !== 'separator') // Filter out separators


idem "divider" instead of separator

AlexVelezLl · 2024-12-11T14:24:58Z

lib/KListWithOverflow.vue

  .list-wrapper {
    display: flex;
-    justify-content: space-between;
+    justify-content: space-between; 


Lets add a alignItems: center instruction here, so the default behaviour is that the items are centered

…tem into new-change3

This reverts commit 4ada24c.

shruti862 · 2024-12-12T13:10:54Z

@AlexVelezLl
I have fixed the keyboard navigation problem and alignment of list items when there is no more button in KListWithOverflow .
Now I am only facing problem in KBreadcrumb , my last visible item of list is getting outside of box even if there is enough space
left in list-wrapper box. Can you please help me resolving this issue?

One more thing I noticed that list items having links differ from whats in the https://design-system.learningequality.org/kbreadcrumbs/ page ,

shruti862 · 2024-12-12T13:45:49Z

when I mounted the items array , link is present on list tems which is not in the https://design-system.learningequality.org/kbreadcrumbs/ page

AlexVelezLl

Hi @shruti862! Thanks! This is looking so much better! I have made some notes that will help to solve the issues that you are experiencing :).

Apologies for the late response 😅.

AlexVelezLl · 2025-01-03T12:33:22Z

lib/KListWithOverflow.vue

-        </slot>
-        <!-- @slot Slot responsible of rendering each item in the visible list -->
+        ></slot>
+        <!-- Item Slot -->


Lets gonna revert this change, this comment is useful for KDS docs:

Suggested change

AlexVelezLl · 2025-01-03T12:37:06Z

lib/KListWithOverflow.vue

+  }
+
+  /* When the 'start-button' class is added, position visually at the start */
+  .more-button-wrapper.start-button {


I dont think this change is needed anymore, is it?

AlexVelezLl · 2025-01-03T12:53:55Z

lib/KListWithOverflow.vue

@@ -153,15 +177,13 @@
          const itemSize = this.getSize(item);


The issue you are facing with the list width not being calculated correctly is because this getSize method currently doesnt support items with margins. So to fix this, lets rewrite this getSize method to something like this:

getSize(element) { if (!element) { return { width: 0, height: 0 }; } let { width, height } = element.getBoundingClientRect(); const style = element.currentStyle || window.getComputedStyle(element); const marginX = parseFloat(style.marginLeft) + parseFloat(style.marginRight); const marginY = parseFloat(style.marginTop) + parseFloat(style.marginBottom); width += marginX; height += marginY; return { width, height }; },

After this you wont see the list item in the next line when there is still space for it..

AlexVelezLl · 2025-01-03T16:49:45Z

lib/KBreadcrumbs.vue

+            :to="item.link"
+          >
+
+            <template #text="{ text }">


Be aware that KRouterLink doesnt have a #text slot. So we just need to use the default slot here instead

AlexVelezLl · 2025-01-03T17:02:42Z

lib/KListWithOverflow.vue

@@ -153,15 +177,13 @@
          const itemSize = this.getSize(item);
          itemsSizes.push(itemSize);
        }
-
+        const indexSequence = [...Array(list.children.length).keys()];


There are a couple of things that needs to be changed with this reversed behaviour:

The fixDividersVisibility method. The intention of this method is to avoid that a divider is placed as the first or last item in the list. And here we calculate this lastVisibleIdx https://github.com/shruti862/kolibri-design-system/blob/01aededc7ac60f49278ebf9ff15db89d8aec0581/lib/KListWithOverflow.vue#L248 which will need to change when we are in reversed mode because the lastVisibleIdx is not the same in both cases.

We have a couple of little bugs here and here, because we change the visibility of the item, but we need to change also the position to be position: absolute if the item is hidden, and position: unset when the item is visible. So, althoguh its not a regression of your PR, fixing this will prevent some bugs that become more apparent when we work with the overflowDirection as "start".

AlexVelezLl · 2025-01-03T17:10:10Z

lib/KBreadcrumbs.vue

+  .breadcrumbs-divider {
+    margin-right: 8px;
+    margin-left: 8px;
+    color: #000000;


Lets avoid burned color, and lets use the KDS color tokens instead https://design-system.learningequality.org/colors/#tokens-text

AlexVelezLl · 2025-01-03T17:10:24Z

lib/KBreadcrumbs.vue

+    padding: 16px;
+    font-size: 16px;
+    font-weight: bold;
+    color: #4368f5;


idem, lets avoid burned colors

AlexVelezLl · 2025-01-03T17:10:35Z

lib/KBreadcrumbs.vue

-    left: -1000em;
+    padding: 16px;
+    font-size: 16px;
+    color: #000000;


updated lib/KListWithOverflow.vue and lib/KBreadcrumbs.vue

cf05645

MisRob marked this pull request as draft December 9, 2024 04:41

added comments to reference issues in lib/KBreadcrumbs.vue

cd730f1

MisRob requested a review from AlexVelezLl December 9, 2024 08:54

MisRob added the TODO: needs review Waiting for review label Dec 9, 2024

AlexVelezLl requested changes Dec 11, 2024

View reviewed changes

new commits

77c86a0

shruti862 closed this Dec 12, 2024

shruti862 force-pushed the new-change3 branch from 77c86a0 to 6c6b208 Compare December 12, 2024 11:54

shruti862 added 2 commits December 12, 2024 17:50

new commit

cfa0321

Merge branch 'new-change3' of github.com:shruti862/kolibri-design-sys…

e3b1804

…tem into new-change3

shruti862 reopened this Dec 12, 2024

shruti862 added 2 commits December 12, 2024 18:18

new commit

4ada24c

Revert "new commit"

01aeded

This reverts commit 4ada24c.

AlexVelezLl requested changes Jan 3, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

updated lib/KListWithOverflow.vue and lib/KBreadcrumbs.vue #857

updated lib/KListWithOverflow.vue and lib/KBreadcrumbs.vue #857

shruti862 commented Dec 8, 2024

MisRob commented Dec 9, 2024

MisRob commented Dec 9, 2024

MisRob commented Dec 9, 2024

shruti862 commented Dec 9, 2024

AlexVelezLl commented Dec 10, 2024

AlexVelezLl left a comment

AlexVelezLl Dec 11, 2024

AlexVelezLl Dec 11, 2024

AlexVelezLl Dec 11, 2024

AlexVelezLl Dec 11, 2024

AlexVelezLl Dec 11, 2024

AlexVelezLl Dec 11, 2024

AlexVelezLl Dec 11, 2024

AlexVelezLl Dec 11, 2024

AlexVelezLl Dec 11, 2024

AlexVelezLl Dec 11, 2024

shruti862 commented Dec 12, 2024 •

edited

Loading

shruti862 commented Dec 12, 2024

AlexVelezLl left a comment

AlexVelezLl Jan 3, 2025

AlexVelezLl Jan 3, 2025

AlexVelezLl Jan 3, 2025

AlexVelezLl Jan 3, 2025

AlexVelezLl Jan 3, 2025

AlexVelezLl Jan 3, 2025

AlexVelezLl Jan 3, 2025

AlexVelezLl Jan 3, 2025

		const itemWidth = itemsSizes[i].width;

		// If the item dont fit in the available space or if we have already

	<!-- Item Slot -->
	<!-- @slot Slot responsible of rendering each item in the visible list -->

updated lib/KListWithOverflow.vue and lib/KBreadcrumbs.vue #857

Are you sure you want to change the base?

updated lib/KListWithOverflow.vue and lib/KBreadcrumbs.vue #857

Conversation

shruti862 commented Dec 8, 2024

Description

Issue addressed

Before/after screenshots

Changelog

Steps to test

(optional) Implementation notes

At a high level, how did you implement this?

Does this introduce any tech-debt items?

Testing checklist

Reviewer guidance

Comments

MisRob commented Dec 9, 2024

MisRob commented Dec 9, 2024

MisRob commented Dec 9, 2024

shruti862 commented Dec 9, 2024

AlexVelezLl commented Dec 10, 2024

AlexVelezLl 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

shruti862 commented Dec 12, 2024 • edited Loading

shruti862 commented Dec 12, 2024

AlexVelezLl 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

shruti862 commented Dec 12, 2024 •

edited

Loading