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

[shruti862]

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

1. Step 1
2. Step 2
3. ...

(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]

Converting to draft #693 (comment)

[MisRob]

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

[MisRob]

@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]

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]

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]

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]

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]

Lets remove these comments, as the code itself is descriptive

[AlexVelezLl]

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

[AlexVelezLl]

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]

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]

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

[AlexVelezLl]

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]

idem "divider" instead of separator

[AlexVelezLl]

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

[shruti862]

@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]

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]

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

Suggested change

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

[AlexVelezLl]

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

[AlexVelezLl]

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]

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

[AlexVelezLl]

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

1. 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.
2. 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]

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

[AlexVelezLl]

idem, lets avoid burned colors

[AlexVelezLl]

Idem

[shruti862]

@AlexVelezLl Can you please review my current changes specially the fixDividersVisibility method ::Since the lastVisibleIdx in case of overflowdirection:'start' will always be last element of list.children because overflowIdx array is calculated after reversing the list.children for start direction so I didn't applied any checks on it.

[shruti862]

@AlexVelezLl But now i am facing issue on truncating the last visible item of the list. Could you please guide me how to approach this??
Right now the output is looking something like this::

[AlexVelezLl]

Hey @shruti862! Apologies for the late response, this is a big change and needed to find a considerable amount of time to review it well. I have left some suggestions here to fix the problems you are facing, and also some considerations we need to take into account. Let me know if you have any question 🤗.

[AlexVelezLl]

instead of a div, we will need to have here a <nav instead, just as it was before.

[AlexVelezLl]

We also need another change here. In the past, the list container was an ol`` element, so we need to modify KListWithOverflowto receive a prop, lets saylistElement` or so, so we can indicate to the KListWithOverflow that the list should be an ol, so we can have the semantic structure ol > li

[AlexVelezLl]

To fix the divider visibility of the overflowed items, we dont need any change actually 😅, so we can just have:

        const [firstOverflowedIdx] = overflowItemsIdx;
        if (this.isDivider(this.items[firstOverflowedIdx])) {
          overflowItemsIdx.shift();
        }

[AlexVelezLl]

Here is where we need to do the change, because when we have an overflowDirection == start, then we will need to check the visibility of the first item instead. So we can do this instead:

if (this.overflowDirection === 'start') {
  const firstVisibleIdx = firstOverflowedIdx + 1;
  if (this.isDivider(this.items[firstVisibleIdx])) {
    const dividerNode = list.children[firstVisibleIdx];
    dividerNode.style.visibility = 'hidden';
    dividerNode.style.position = 'absolute';
    return itemsSizes[firstVisibleIdx].width;
  }
} else {
  const lastVisibleIdx = firstOverflowedIdx - 1;
  if (this.isDivider(this.items[lastVisibleIdx])) {
    const dividerNode = list.children[lastVisibleIdx];
    dividerNode.style.visibility = 'hidden';
    return itemsSizes[lastVisibleIdx].width;
  }
}

In general this method should look like:

      fixDividersVisibility(overflowItemsIdx, itemsSizes) {
        if (overflowItemsIdx.length === 0) {
          return;
        }

        const { list } = this.$refs;

        const [firstOverflowedIdx] = overflowItemsIdx;
        if (this.isDivider(this.items[firstOverflowedIdx])) {
          overflowItemsIdx.shift();
        }

        if (this.overflowDirection === 'start') {
          const firstVisibleIdx = firstOverflowedIdx + 1;
          if (this.isDivider(this.items[firstVisibleIdx])) {
            const dividerNode = list.children[firstVisibleIdx];
            dividerNode.style.visibility = 'hidden';
            dividerNode.style.position = 'absolute';
            return itemsSizes[firstVisibleIdx].width;
          }
        } else {
          const lastVisibleIdx = firstOverflowedIdx - 1;
          if (this.isDivider(this.items[lastVisibleIdx])) {
            const dividerNode = list.children[lastVisibleIdx];
            dividerNode.style.visibility = 'hidden';
            return itemsSizes[lastVisibleIdx].width;
          }
        }
      },

[AlexVelezLl]

To avoid messing with the dividers visibility after the KIconButton, we can just add the divider besides the KIconButton from the more slot, since for this case, always that we show the more slot, will need to show the divider. So we can have just something like

      <template #more="{ overflowItems }">
        <div style="display: flex; align-items: center;">
          <KIconButton
            ...
          >
            ...
          </KIconButton>
          <span
            :style="{ color: $themeTokens.text }"
            class="breadcrumbs-divider"
          >›
          </span>
        </div>
      </template>

[AlexVelezLl]

Be aware that the last breadcrumb should not be a link, but just a text.

Before:

After:

Here, Level 2 shouldnt be a link

[AlexVelezLl]

We will also need to set the maxWidth that we set to the spans here to the li, for it to know what its its own max width, instead of getting it from the listOverflow container.

[AlexVelezLl]

doesnt have a slot with props, and it doesnt pass the text to the slot, and this is making that we dont render the span with the breadcrumbs-crumb-text class. We need to have the slot defined just as <template #default> and inside render the text using item.text