Lundalogik / lime-elements

Provides reusable web components for Lime CRM
https://lundalogik.github.io/lime-elements/versions/latest
Other
38 stars 12 forks source link

limel-list may show deselected items as selected #1077

Open anderssonjohan opened 3 years ago

anderssonjohan commented 3 years ago

Current behavior

The limel-list component uses MDCList internally, which adds styles to the DOM to indicate list selection. When StencilJS caches the DOM element and you render a completely different limel-list, the list selection from the first list "sticks" due to the fact of reused DOM elements. Even though the item seems selected visually, it's only in the DOM and not in the bound list of items. This means that the selection can't be cleared by deleting the selected property on all items, while re-setting the items.

Gif showing two data sources where the selection visually sticks because of the reused DOM elements.

Kapture 2020-12-15 at 16 32 58

Workaround

Example: The GIF above used different lists when the tab-bar component was changed. Therefore, the limel-list used in the example could be fixed by adding the following key property:

<limel-list key={`recentitems-${this.activeTab.id}`} .../>

...where this.activeTab.id is a unique key to identify each tab.

The downside with the workaround is that it would re-create the whole list component instead of just recreating the <li/> elements that needs to be updated.

Suggested solution

To force StencilJS to replace the DOM elements when needed, the limel-list should add a key attribute to each <li>. Current code: https://github.com/Lundalogik/lime-elements/blob/5d4503a3757777b20e5d39bdd2bfedad587f8fa1/src/components/list/list-renderer.tsx#L278-L285

Possible solution:

<li
   class={classNames}
   role={config.type}
   aria-checked={item.selected ? 'true' : 'false'}
   aria-disabled={item.disabled ? 'true' : 'false'}
   data-index={index}
   key={'id' in item ? item.id : index}
   {...attributes}
>

That would allow the current list to be reused when needed (like when the user clicks "show more results" and the current DOM elements shouldn't be recreated). By using key={index} will result in today's behavior so the real benefit is when you can use key={item.id}, where the id here could be anything like a uuid, a object hash, {limeobject.limetype}:{limeobject.id} etc.

More info on the key="..." attribute from the docs:

Expected behavior

Environment

adrianschmidt commented 3 years ago

If I recall correctly, the Stencil Docs warn against using key={index} so it may be better to just set a random value if there is no id, and thus (presumably) force recreation of the nodes each time… it may also be much much worse of course… just floating ideas here 😄