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

Refactor KBreadcrumbs to utilize KListWithOverflow #693

Open
1 task
MisRob opened this issue Jul 26, 2024 · 17 comments
Open
1 task

Refactor KBreadcrumbs to utilize KListWithOverflow #693

MisRob opened this issue Jul 26, 2024 · 17 comments
Assignees

Comments

@MisRob
Copy link
Member

MisRob commented Jul 26, 2024

🌱 Are you new to the codebase? Welcome! Please see the contributing guidelines.

In times when KListWithOverflow component didn't yet exist, we used the "duplicate markup" technique in the KBreadcrumbs component to achieve their overflow behavior when breadcrumb items were collapsed to the dropdown menu on the left:

breadcrumbs

The duplicate markup technique is based on keeping a hidden copy of visible breadcrumbs (breadcrumbs-offscreen) in the markup so that elements are available for recalculations:

<!-- This is a duplicate of breacrumbs-visible-items to help to reference sizes. -->
<div class="breadcrumbs breadcrumbs-offscreen" aria-hidden="true">
<ol class="breadcrumbs-visible-items">
<template v-for="(crumb, index) in crumbs">
<li
v-if="index !== crumbs.length - 1"
:ref="`crumb${index}`"
:key="index"
class="breadcrumbs-visible-item breadcrumbs-visible-item-notlast"
>
<KRouterLink v-if="crumb.link" :text="crumb.text" :to="crumb.link" tabindex="-1">
<template #text="{ text }">
<span class="breadcrumbs-crumb-text">{{ text }}</span>
</template>
</KRouterLink>
<span v-else>{{ crumb.text }}</span>
</li>
<li
v-else
:ref="`crumb${index}`"
:key="index"
class="breadcrumb-visible-item-last breadcrumbs-visible-item"
>
<span
class="breadcrumbs-crumb-text"
:style="{ maxWidth: lastBreadcrumbMaxWidth }"
>
{{ crumb.text }}
</span>
</li>
</template>
</ol>
</div>
</div>

Now that we have KListWithOverflow that is designed to take care of overflowing items to a menu, it would be good to remove this other implementation from KBreadcrumbs and refactor them to utilize KListWithOverflow instead.

The Value Add

Easier maintenance, development efficiency.

Guidance

Note that KListWithOverflow will likely need some updates to be able to display the button on the left side, and possibly some other adjustments so that these two components can collaborate smoothly.

Acceptance criteria

  • There are no regressions in places where KBreadcrumbs and KListWithOverflow are used:
    • Live examples on their documentation pages
    • In Kolibri and Studio
@MisRob MisRob added help wanted Open source contributors welcome Component: KBreadcrumbs Component: KListWithOverflow P3 - low Priority: Stretch goal labels Jul 26, 2024
@sruthin21
Copy link

Hey @MisRob I want to work on this Issue Please Assign this to me

@AlexVelezLl
Copy link
Member

Hey @sruthin21! Thank you! I just assigned you this issue. Please let us know if you have any question. I was thinking to propose something for this issue but probably will be able to do that tomorrow :). But you can start looking at the issue for now.

@sruthin21
Copy link

@AlexVelezLl Thank You for assigning

@AlexVelezLl
Copy link
Member

Hey @sruthin21! I am back with some things we will need for this:

  1. We need to update KListWithOverflow since now we need to allow hiding overflowed items from the beginning, instead of from the end of the list as we courrently do. For this:
  • We need to define a new prop overflowDirection that accepts as values start or end, with end as default value as this would be the current behaviour.
  • We need to position the more button at the beginning or at the end of the list, depending on the overflowDirection prop.
  • We need to modify the hiding elements logic inside the setOverflowItems method to support hiding elements from the beginning. This is probably the biggest challenge of the issue. I suggest to try to look for mathematical operations to achieve the reverse overflow instead of duplicating the logic for both overflowDirection values.
  1. Update KBreadcrums to use KListWithOverflow.
  • To prepare the items for this, we will need to add a { type: "separator" } object between every element, and use the #divider slot to render the "> " icon.
  • We have this fixDividerVisibility method in KListWithOverflow to avoid having a separator at the beginning or at the end, and this will mess with the requirement of KBreadcrums to have the ">" icon just after the more button, so the easiest way to achieve that will be that the #more slot renders both, the more button, and the ">" separator icon. With this, we will avoid having to care about that.

There will problably be a lot of things that I am missing, so please let us know if you have any questions 🤗.

@sruthin21
Copy link

What this throttle function do And what will be the output of the function

@AlexVelezLl
Copy link
Member

AlexVelezLl commented Sep 17, 2024

This is just a way to optimize the number of calls we do to the setOverflowItems method, you can ignore it, and assume we will be always calling setOverflowItem each time the parent element is resized. You can read more about thottling here https://dev.to/jeetvora331/throttling-in-javascript-easiest-explanation-1081.

@sruthin21
Copy link

sruthin21 commented Sep 23, 2024

@AlexVelezLl I was able to fix the first task like adding the overflowDirection prop to the KListWithOverflow Componenet
But unable to resolve the second task That is Updating KBreadcrums to use KListWithOverflow Component
Can you give the Idea to solve the second task
And please tell me what the final output of the KBreadcrums component should look like

@AlexVelezLl
Copy link
Member

AlexVelezLl commented Sep 23, 2024

Hey @sruthin21! Could you elaborate further on what problems you have experienced with the second point? The component should look more or less like this:

<!-- KBreadcrumbs.vue -->
<KListWithOverflow
  overflowDirection="start"
  :items="crumbs"
>
  <template #item="{ item }">
    <li>
      <KRouterLink
        v-if="item.link"
        :text="item.text"
        :to="item.link"
      >
        <template #text="{ text }">
          <span class="breadcrumbs-crumb-text">{{ text }}</span>
        </template>
      </KRouterLink>
      <span v-else>{{ item.text }}</span>
    </li>
  </template>
  <template #more="{ overflowItems }">
    <KIconButton
      size="small"
      icon="chevronUp"
      appearance="raised-button"
    >
      <template #menu>
        <KDropdownMenu
          :options="overflowItems"
        />
      </template>
    </KIconButton>
    <span>
      ›
    </span>
  </template>
</KListWithOverflow>

So the #item template should match the current visible breadcrumbs items, and we can use our dropodownmenu in the #more template. Having something like this in the template of KBreadcrumbs is the goal.

And please tell me what the final output of the KBreadcrums component should look like

For this you can see the Acceptance creteria in the issue description:

There are no regressions in places where KBreadcrumbs and KListWithOverflow are used:
* Live examples on their documentation pages
* In Kolibri and Studio

So basically as this is a refactor, the output should look exactly the same as it is right now, and we should make sure that we dont introduce bugs on something there was already working ok. We can check regressions in studio, and you can do it in Kolibri and in the docs pages. to test your changes in Kolibri you can check the how to preview updates in a product guide.

@sruthin21
Copy link

@AlexVelezLl I have made a pull request review it I know there are lots of changes to do

@MisRob
Copy link
Member Author

MisRob commented Dec 2, 2024

Note the conversation above and the first attempt #790. Quite detailed review by @AlexVelezLl will serve as good guidance

@shruti862
Copy link
Contributor

Hey @MisRob I want to work on this issue. Please Assign this to me

@MisRob
Copy link
Member Author

MisRob commented Dec 6, 2024

Hi @shruti862, sure! Thank you. Note this is a bit more complex, but we already had one attempt and it was reviewed, so please check out the comments above as guidance.

@shruti862
Copy link
Contributor

@MisRob I am done with the task-1 of the issue but i am facing issue in refactoring KBreadcrumb with updated KListWithOverflow ; I am unable to render '>' separator between items and to insert link in the text of dropdown menu in KBreadcrumb . Could you please review my code so that i get to know what i am doing wrong.I am raising a PR soon.

@MisRob
Copy link
Member Author

MisRob commented Dec 9, 2024

Hi @shruti862, thanks! I converted your pull request to draft until it's finalized.

i am facing issue in refactoring KBreadcrumb with updated KListWithOverflow ; I am unable to render '>' separator between items and to insert link in the text of dropdown menu in KBreadcrumb

In your pull request, would you comment on some particular places that are related to this and be as specific as possible? We will need to hear more to understand what you're experiencing.

@shruti862
Copy link
Contributor

shruti862 commented Dec 9, 2024

@MisRob yes sure . I am facing issue in lib/KBreadcrumb.vue file ,I will make comments on this file regarding the issues i am facing.
But can you please review KListWithOverflow to see if it is meeting all the requirements. In that file I tried to include all suggested changes AlexVelezLl suggested in referenced PR.

@MisRob
Copy link
Member Author

MisRob commented Dec 9, 2024

Great @shruti862. I will invite @AlexVelezLl to assist there. The more information you can provide in the draft PR, the better. Thanks a lot for this work!

@MisRob
Copy link
Member Author

MisRob commented Dec 17, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants