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.

Sign up for GitHub

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

More icon interface #2688

Merged
merged 4 commits into from
Jan 4, 2024
Merged

More icon interface #2688

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3e97bd7
fix(list): avoid rendering icon when icon.name is undefined
Befkadu1 Jan 4, 2024
e30c400
fix(menu): avoid rendering icon when icon.name is undefined
Befkadu1 Jan 4, 2024
61713cf
fix(header): make sure icon name exist before rendering the icon
Befkadu1 Jan 4, 2024
b34746c
refactor(picker): use the new icon interface
Befkadu1 Jan 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/components/header/header.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,12 +103,12 @@ export class Header {
}

private renderIcon() {
if (!this.icon) {
const icon = getIconName(this.icon);

if (!icon) {
return;
}

const icon = getIconName(this.icon);

return <limel-icon class="icon" badge={true} name={icon} />;
}

Expand Down
8 changes: 6 additions & 2 deletions src/components/list/list-renderer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ export class ListRenderer {
data-index={index}
{...attributes}
>
{item.icon ? this.renderIcon(this.config, item) : null}
{this.renderIcon(this.config, item)}
{this.getPrimaryComponent(item)}
{this.renderText(item)}
{this.twoLines && this.avatarList ? this.renderDivider() : null}
Expand Down Expand Up @@ -211,11 +211,15 @@ export class ListRenderer {
* Render an icon for a list item
* @param {ListRendererConfig} config the config object, passed on from the `renderListItem` function
* @param {ListItem} item the list item
* @returns {HTMLElement} the icon element
* @returns {HTMLElement | undefined} the icon element
*/
private renderIcon = (config: ListRendererConfig, item: ListItem) => {
const style: any = {};
const name = getIconName(item.icon);
if (!name) {
return;
}

const color = getIconColor(item.icon, item.iconColor);

if (color) {
Expand Down
8 changes: 6 additions & 2 deletions src/components/menu-list/menu-list-renderer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ export class MenuListRenderer {
data-index={index}
{...attributes}
>
{item.icon ? this.renderIcon(this.config, item) : null}
{this.renderIcon(this.config, item)}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😬

This change makes the list crash if any item doesn't have icon specified 😞

Reports of several customers having this problem: https://forum.lime-crm.com/t/query-visualizer-not-working-with-newer-lime-elements/5723

The problem is that renderIcon uses getIconName, which in turn does:

if (typeof icon === 'object' && 'name' in icon) {
    return icon.name;
}

The problem is that typeof null returns 'object', so this tries to run 'name' in null, which causes an error.

I think get-icon-props.ts is a pretty perfect example of how helper-functions can be broken out into a separate file, to make them testable. But unfortunately, there are no tests for that file 🙈 😄

I'll look into adding some tests and fixing the bug.

@Befkadu1 @Kiarokh

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙏

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

omg I did not know that typeof null returns 'object' 🙈. Sorry for the bug we created! Thanks for letting us know.

{this.renderText(item)}
{this.renderSubMenuIcon(item)}
{this.renderNotification(item)}
Expand Down Expand Up @@ -213,11 +213,15 @@ export class MenuListRenderer {
* Render an icon for a list item
* @param {MenuListRendererConfig} config the config object, passed on from the `renderMenuItem` function
* @param {MenuItem} item the list item
* @returns {HTMLElement} the icon element
* @returns {HTMLElement | undefined} the icon element
*/
private renderIcon = (config: MenuListRendererConfig, item: MenuItem) => {
const style: any = {};
const name = getIconName(item.icon);
if (!name) {
return;
}

const color = getIconColor(item.icon, item.iconColor);

if (color) {
Expand Down
9 changes: 4 additions & 5 deletions src/components/picker/picker.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -321,15 +321,14 @@ export class Picker {
}

private createChip(listItem: ListItem): Chip {
const name = getIconName(listItem.icon);
const color = getIconFillColor(listItem.icon, listItem.iconColor);

return {
id: `${listItem.value}`,
text: listItem.text,
removable: true,
icon: name,
iconFillColor: color,
icon: {
name: getIconName(listItem.icon),
color: getIconFillColor(listItem.icon, listItem.iconColor),
},
value: listItem,
};
}
Expand Down