From de1de6ca77a669fafa5d73f7dccd7d960b29a2d1 Mon Sep 17 00:00:00 2001 From: Josh Winn <965114+jawinn@users.noreply.github.com> Date: Thu, 20 Feb 2025 16:34:24 -0500 Subject: [PATCH 01/11] feat(icon): simplify fallback of ui icon size numbers If a UI icon was not provided with its full name including sizing number (e.g. Dash100), the template would try and guess at a sizing number based on t-shirt size. This has been extracted out to a utility function and simplified, as this had gotten too complex, with too many exceptions. It no longer strips out any sizing numbers that were specifically provided. docs(icon): log error when icon is not in the icon set Update icon Storybook template with a guard clause so that it logs a warning and returns when the icon is not in the icon set. Previously there was no indication except for not seeing the icon visually. Also renames "idKey" variable to better explain what it is. --- components/icon/stories/template.js | 136 ++++++++------------------- components/icon/stories/utilities.js | 24 +++++ 2 files changed, 61 insertions(+), 99 deletions(-) diff --git a/components/icon/stories/template.js b/components/icon/stories/template.js index b6deef70de5..0538ba2ea69 100644 --- a/components/icon/stories/template.js +++ b/components/icon/stories/template.js @@ -6,7 +6,7 @@ import { ifDefined } from "lit/directives/if-defined.js"; import { styleMap } from "lit/directives/style-map.js"; import { unsafeSVG } from "lit/directives/unsafe-svg.js"; import { when } from "lit/directives/when.js"; -import { getSpriteSheetName, uiIconsCleaned, uiIconsWithDirections, workflowIconsCleaned } from "./utilities.js"; +import { appendUiIconDefaultSizing, getSpriteSheetName, uiIconsWithDirections, workflowIconsCleaned } from "./utilities.js"; import "../index.css"; @@ -33,7 +33,7 @@ import "../index.css"; export const Template = ({ rootClass = "spectrum-Icon", size = "m", - setName, + setName = "workflow", iconName, uiIconName, fill, @@ -50,113 +50,51 @@ export const Template = ({ iconName = uiIconName; } - // Make sure icon name is provided. - if (!iconName) { + // Make sure icon set is provided. + if (!["ui","workflow"].includes(setName)) { console.warn( - "Icon: Could not render a result because no icon name was provided to the icon template." + `Icon "${iconName}" is missing its icon set. Make sure you are explicitly setting either the workflow or ui icon set.` ); return html``; } - // Name of icon that corresponds with SVG file. This may differ from the icon name, such as with - // directional icons that use a single icon. - let idKey = iconName; - - // If a descriptor like "Right", "Left", "Down", or "Up" is present for the UI icons Chevron or - // Arrow, use that only for the class name and not the icon fetch. This is because these use a - // single icon file that is rotated in CSS. - if ( - ["Right", "Left", "Down", "Up"].some((c) => idKey.includes(c)) && - setName === "ui" - ) { - idKey = idKey.replace(/(Right|Left|Down|Up)/, ""); - } - - // Make sure icon set is provided. - if (!setName) { - console.warn( - `Icon "${idKey}" is missing its icon set. Make sure you are explicitly setting either the workflow or ui icon set.` - ); + // Make sure icon name is provided. + if (!iconName) { + console.warn("Icon: Could not render a result because no icon name was provided to the icon template."); return html``; } /** - * Fallback UI Icon sizing number. - * - * If the icon name includes its scale, we want to leave that scale. This is preferred, - * as UI icons do not use workflow icon sizing. + * Append approximate sizing number to UI icons passed in without a sizing number. * - * If the UI icon name does not include scale, or the scale does not exist in the current - * list of UI icons, reformat it to approximate the provided sizing for the component. + * Note: It's preferred for components to provide the specific UI sizing numbers in the UI icon + * name, rather than relying on this approximation, as UI icons do not use t-shirt sizing. */ + if (setName === "ui") { + iconName = appendUiIconDefaultSizing(iconName, size); + } + + // Make sure icon exists in the set. if ( - setName === "ui" && - ( - // Does not already have size number at the end. - !idKey.match(/\d{2,3}$/) || - // If the provided icon name includes the sizing number, make sure it's a supported sizing number; - // if not, strip it from the key. - ( - idKey.match(/\d{2,3}$/) && - !uiIconsCleaned.includes(idKey) - ) - ) + (setName == "workflow" && !workflowIconsCleaned.includes(iconName)) || + (setName == "ui" && !uiIconsWithDirections.includes(iconName)) ) { - let sizeVal; - switch (size) { - case "xs": - if (["CornerTriangle", "Cross"].some(c => idKey.startsWith(c))) { - sizeVal = "75"; - } - else if (["Arrow", "Asterisk", "LinkOut"].some(c => idKey.startsWith(c))) { - sizeVal = "100"; - } - else { - sizeVal = "50"; - } - break; - case "s": - if (["Arrow", "Asterisk", "LinkOut"].some(c => idKey.startsWith(c))) { - sizeVal = "100"; - } - else { - sizeVal = "75"; - } - break; - case "l": - if (["Arrow"].some(c => idKey.startsWith(c))) { - sizeVal = "400"; - } - else { - sizeVal = "200"; - } - break; - case "xl": - case "xxl": - if (["Arrow"].some(c => idKey.startsWith(c))) { - sizeVal = "400"; - } - else { - sizeVal = "300"; - } - break; - default: - sizeVal = "100"; - break; - } - - console.warn(`Using fallback UI Icon sizing number "${sizeVal}" for "${idKey}". UI icon size was not provided or does not exist in the list of available UI icons.`); - - // Replace sizing number on idKey and iconName with new fallback size. - idKey = idKey.replace(/\d{2,3}$/, ""); - idKey += sizeVal; + console.warn(`Icon: Could not render an icon with the name "${iconName}" because it does not exist in the ${setName} icon set.`); + return html``; + } - iconName = iconName.replace(/\d{2,3}$/, ""); - iconName += sizeVal; + // Name of icon that corresponds with SVG file. This may differ from the icon name, such as with + // directional icons that use a single icon. + let iconNameToLoad = iconName; - if (!uiIconsCleaned.includes(idKey)) { - console.error(`The UI icon "${idKey}" does not exist in the list of available UI icons.`); - } + // If a descriptor like "Right", "Left", "Down", or "Up" is present for the UI icons Chevron or + // Arrow, use that only for the class name and not the icon fetch. This is because these use a + // single icon file that is rotated in CSS. + if ( + ["Right", "Left", "Down", "Up"].some((c) => iconNameToLoad.includes(c)) && + setName === "ui" + ) { + iconNameToLoad = iconNameToLoad.replace(/(Right|Left|Down|Up)/, ""); } /** @@ -178,8 +116,8 @@ export const Template = ({ */ if (!useRef) { let svgString; - if (loaded?.icons && loaded?.icons[setName]?.[idKey]) { - svgString = loaded.icons[setName][idKey]; + if (loaded?.icons && loaded?.icons[setName]?.[iconNameToLoad]) { + svgString = loaded.icons[setName][iconNameToLoad]; } // Return the individual SVG's entire markup. @@ -194,12 +132,12 @@ export const Template = ({ )}`; } else { - console.warn(`Could not find SVG markup for "${idKey}" in context.loaded.icons. Did you pass through context? Falling back to using the sprite sheet reference instead.`); + console.warn(`Could not find SVG markup for "${iconNameToLoad}" in context.loaded.icons. Was context passed through in the template? Falling back to using the sprite sheet reference instead.`); } } // ID of the icon within the sprite sheet for its icon set. - const iconID = getSpriteSheetName(idKey, setName); + const iconID = getSpriteSheetName(iconNameToLoad, setName); // Return SVG markup with a reference to the icon ID within the sprite sheet. return html` - ${idKey.replace(/([A-Z])/g, " $1").trim()} + ${iconNameToLoad.replace(/([A-Z])/g, " $1").trim()} `; }; diff --git a/components/icon/stories/utilities.js b/components/icon/stories/utilities.js index 2ba609a3416..16faf30eaa3 100644 --- a/components/icon/stories/utilities.js +++ b/components/icon/stories/utilities.js @@ -190,3 +190,27 @@ export const getUiIconSizes = (uiIcons) => { }; export const uiIconSizes = getUiIconSizes(uiIconsWithDirections); + +/** + * If UI icon name does not have a sizing number appended, add one to approximate the provided + * t-shirt sizing for the component, based on the most common mapping. + * + * @param {string} uiIconName + * @param {string} size t-shirt sizing + * @returns {string} uiIconName with appended default sizing number, if one is not already present. + */ +export const appendUiIconDefaultSizing = (uiIconName, size = "m") => { + // If icon name already has a size number on the end, no change is needed. + if (uiIconName.match(/\d{2,3}$/)) { + return uiIconName; + } + + return uiIconName + ({ + xs: "50", + s: "75", + m: "100", + l: "200", + xl: "300", + xxl: "400", + }[size] || "100"); +}; From 82fbccc132e5e895496a1d323f7917041f3270e6 Mon Sep 17 00:00:00 2001 From: Josh Winn <965114+jawinn@users.noreply.github.com> Date: Fri, 21 Feb 2025 13:35:04 -0500 Subject: [PATCH 02/11] fix(menu): update ui icon sizes used in template - The Arrow icon is now only available in 100 and 400; adjusts which arrow number size is displayed for each component t-shirt size. - The iconWithScale function was only the mapping for the arrow and was incorrectly used for the other icons like chevron and checkmark. --- components/menu/stories/template.js | 30 ++++++++--------------------- 1 file changed, 8 insertions(+), 22 deletions(-) diff --git a/components/menu/stories/template.js b/components/menu/stories/template.js index 79c4136fdad..9c8bea73cfe 100644 --- a/components/menu/stories/template.js +++ b/components/menu/stories/template.js @@ -14,22 +14,6 @@ import { when } from "lit/directives/when.js"; import "../index.css"; -/** - * Get the tray submenu back arrow name with scale number (defined in design spec). - */ -const iconWithScale = (size = "m", iconName = "ArrowLeft") => { - switch (size) { - case "s": - return `${iconName}200`; - case "l": - return `${iconName}400`; - case "xl": - return `${iconName}500`; - default: - return `${iconName}300`; - } -}; - export const MenuItem = ( { rootClass = "spectrum-Menu-item", @@ -84,10 +68,7 @@ export const MenuItem = ( ${when(isCollapsible || (selectionMode == "single" && isSelected), () => Icon( { - iconName: iconWithScale( - size, - isCollapsible ? "ChevronRight" : "Checkmark", - ), + iconName: isCollapsible ? "ChevronRight" : "Checkmark", setName: "ui", useRef: false, size, @@ -196,7 +177,7 @@ export const MenuItem = ( ${when(isDrillIn, () => Icon( { - iconName: iconWithScale(size, "ChevronRight"), + iconName: "ChevronRight", setName: "ui", useRef: false, size, @@ -266,7 +247,12 @@ export const MenuGroup = ( > ${Icon( { - iconName: iconWithScale(size), + iconName: "ArrowRight" + ({ + s: "100", + m: "100", + l: "400", + xl: "400", + }[size] || "100"), setName: "ui", size, customClasses: ["spectrum-Menu-backIcon"], From a60e0b668d330411b3d61aaa9f3edc896b6ece66 Mon Sep 17 00:00:00 2001 From: Josh Winn <965114+jawinn@users.noreply.github.com> Date: Fri, 21 Feb 2025 13:38:46 -0500 Subject: [PATCH 03/11] docs(button): replace deprecated ui icon size in test The previously referenced UI icon no longer exists in the UI icons package. Replaces with the largest UI icon available. --- components/button/stories/button.test.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/components/button/stories/button.test.js b/components/button/stories/button.test.js index bdc6ee37996..ad83defdf1e 100644 --- a/components/button/stories/button.test.js +++ b/components/button/stories/button.test.js @@ -60,8 +60,8 @@ const ButtonIconGroup = (args, context) => Container({ testHeading: "UI icon (larger)", content: Template({ ...args, - // UI icon that is larger than workflow sizing: - iconName: "ArrowDown600", + // Largest UI icon from UI icon set: + iconName: "Cross600", iconSet: "ui", }, context), }, From 4003b4311e568b375bc8e31e6bd68c856d363d4c Mon Sep 17 00:00:00 2001 From: Josh Winn <965114+jawinn@users.noreply.github.com> Date: Fri, 21 Feb 2025 13:49:39 -0500 Subject: [PATCH 04/11] feat(swatch): use spec defined ui icon size number for dash Use the correct size number for the "Dash" ui icon per t-shirt size of the swatch, according to its S2 design spec. --- components/swatch/stories/template.js | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/components/swatch/stories/template.js b/components/swatch/stories/template.js index c1ff884c4c6..bcf81178f6d 100644 --- a/components/swatch/stories/template.js +++ b/components/swatch/stories/template.js @@ -106,7 +106,12 @@ export const Template = ({ ...(isMixedValue ? [Icon({ customClasses: [`${rootClass}-mixedValueIcon`], setName: "ui", - iconName: "Dash", + iconName: "Dash" + ({ + xs: "75", + s: "75", + m: "100", + l: "200", + }[size] || "100"), useRef: false, }, context)] : []), ] From 58036b2b1c7da3099962f287dc9b4f3af291ca9e Mon Sep 17 00:00:00 2001 From: Josh Winn <965114+jawinn@users.noreply.github.com> Date: Thu, 27 Feb 2025 15:28:44 -0500 Subject: [PATCH 05/11] fix: icon set arg was missing a value in a few places Make sure the iconset arg is set in a few components that are using the Icon template, otherwise it was passed through as undefined. Also set a default icon set in the templates used. --- components/search/stories/template.js | 1 + components/textfield/stories/template.js | 2 +- components/treeview/stories/template.js | 2 +- 3 files changed, 3 insertions(+), 2 deletions(-) diff --git a/components/search/stories/template.js b/components/search/stories/template.js index 09570d33581..d017e28981f 100644 --- a/components/search/stories/template.js +++ b/components/search/stories/template.js @@ -34,6 +34,7 @@ export const Template = ({ size, customClasses: [`${rootClass}-textfield`], iconName: "Magnify", + setName: "workflow", type: "search", placeholder: "Search", name: "search", diff --git a/components/textfield/stories/template.js b/components/textfield/stories/template.js index 9413a7504e3..e255eeae0ee 100644 --- a/components/textfield/stories/template.js +++ b/components/textfield/stories/template.js @@ -75,7 +75,7 @@ export const Template = ({ labelText, characterCount, iconName, - iconSet, + iconSet = "workflow", pattern, placeholder, name, diff --git a/components/treeview/stories/template.js b/components/treeview/stories/template.js index 6c15ccdacd7..7114a75a1ac 100644 --- a/components/treeview/stories/template.js +++ b/components/treeview/stories/template.js @@ -22,7 +22,7 @@ export const TreeViewItem = ({ isOpen, isDropTarget, icon, - iconSet, + iconSet = "workflow", thumbnail, items, variant, From e0eabe9da3b8de0f8b5ccb4e0d48e2f0d8052abb Mon Sep 17 00:00:00 2001 From: Josh Winn <965114+jawinn@users.noreply.github.com> Date: Thu, 27 Feb 2025 16:15:40 -0500 Subject: [PATCH 06/11] fix(actionbutton): define specific sizing for corner triangle We have to define specific sizing for the UI icon corner triangle, becauses there is no size 50 for extra small. Fixes XS action button not showing the corner triangle icon. --- components/actionbutton/stories/template.js | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/components/actionbutton/stories/template.js b/components/actionbutton/stories/template.js index 9c1fd980055..13d353d5306 100644 --- a/components/actionbutton/stories/template.js +++ b/components/actionbutton/stories/template.js @@ -109,7 +109,13 @@ export const Template = ({ ${when(hasPopup && hasPopup !== "false", () => Icon({ size, - iconName: "CornerTriangle", + iconName: "CornerTriangle" + ({ + xs: "75", + s: "75", + m: "100", + l: "400", + xl: "400", + }[size] || "100"), setName: "ui", customClasses: [`${rootClass}-hold`], }, context) From bfafc688df88d020fe489c7f96c0d13c480ec92a Mon Sep 17 00:00:00 2001 From: Josh Winn <965114+jawinn@users.noreply.github.com> Date: Thu, 27 Feb 2025 16:26:57 -0500 Subject: [PATCH 07/11] feat(icon): display placeholder icon when workflow icon is missing Display the design recommended "Circle" icon as a placeholder icon when the workflow icon cannot be found. This helps with identifying missing icons in VRTs, so the icon continues to take up the space it should, rather than disappearing entirely in some cases. --- components/icon/stories/template.js | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/components/icon/stories/template.js b/components/icon/stories/template.js index 0538ba2ea69..8ed1b766205 100644 --- a/components/icon/stories/template.js +++ b/components/icon/stories/template.js @@ -75,14 +75,16 @@ export const Template = ({ } // Make sure icon exists in the set. - if ( - (setName == "workflow" && !workflowIconsCleaned.includes(iconName)) || - (setName == "ui" && !uiIconsWithDirections.includes(iconName)) - ) { - console.warn(`Icon: Could not render an icon with the name "${iconName}" because it does not exist in the ${setName} icon set.`); + if (setName == "ui" && !uiIconsWithDirections.includes(iconName)) { + console.warn(`Icon: Could not render an icon with the name "${iconName}" because it does not exist in the "ui" icon set.`); return html``; } + if (setName == "workflow" && !workflowIconsCleaned.includes(iconName)) { + console.warn(`Icon: Could not render the correct icon with the name "${iconName}" because it does not exist in the "workflow" icon set. Rendering the placeholder icon instead.`); + iconName = "Circle"; + } + // Name of icon that corresponds with SVG file. This may differ from the icon name, such as with // directional icons that use a single icon. let iconNameToLoad = iconName; From 7fa1894b67e05689a041c856ecd753be8953190c Mon Sep 17 00:00:00 2001 From: Josh Winn <965114+jawinn@users.noreply.github.com> Date: Sat, 1 Mar 2025 09:36:33 -0500 Subject: [PATCH 08/11] docs(icon): replace icon names that were removed in 4.0 package --- components/icon/stories/template.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/components/icon/stories/template.js b/components/icon/stories/template.js index 8ed1b766205..75826ee6d40 100644 --- a/components/icon/stories/template.js +++ b/components/icon/stories/template.js @@ -244,7 +244,7 @@ export const WorkflowDefaultTemplate = (args, context) => html` }, context, [ - "AlertCircle", + "AlertTriangle", "Bell", "Camera", "Color", @@ -255,7 +255,7 @@ export const WorkflowDefaultTemplate = (args, context) => html` "Files", "Hand", "Lightbulb", - "Paragraph", + "InfoCircle", ] )} `; From 9adbc78628dc9bf7a07cb743d7cefd0bb354f0d3 Mon Sep 17 00:00:00 2001 From: Josh Winn <965114+jawinn@users.noreply.github.com> Date: Sat, 1 Mar 2025 10:03:41 -0500 Subject: [PATCH 09/11] fix(menu): add missing context to menu templates Add context that was missing from some menu storybook templates, which was causing a warning within Icon. --- components/menu/stories/template.js | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/components/menu/stories/template.js b/components/menu/stories/template.js index 9c8bea73cfe..98b765fc520 100644 --- a/components/menu/stories/template.js +++ b/components/menu/stories/template.js @@ -459,7 +459,7 @@ export const DisabledItemGroup = (args, context) => { context, shouldTruncate: group.shouldTruncate || false, items: group.items, - })} + }, context)} ` }, context)} `) @@ -568,11 +568,11 @@ export const OverflowGroup = (args, context) => { context, shouldTruncate: group.shouldTruncate || false, items: group.items, - })} + }, context)} ` - })} + }, context)} `) - }); + }, context); }; export const SelectionGroup = (args, context) => { @@ -686,12 +686,12 @@ export const SelectionGroup = (args, context) => { selectionMode: group.selectionMode || "none", hasActions: group.hasActions || false, items: group.items, - }) + }, context) }, context)) }); }; -export const SubmenuInPopover = (context) => Popover({ +export const SubmenuInPopover = (args, context) => Popover({ isOpen: true, position: "end-top", customStyles: { @@ -703,7 +703,8 @@ export const SubmenuInPopover = (context) => Popover({ ...args, }, context), content: [ - (args, context) => Template({ + Template({ + ...args, items: [ { label: "Language", @@ -718,9 +719,8 @@ export const SubmenuInPopover = (context) => Popover({ label: "Show grid", } ], - ...args }, context), - (args, context) => Popover({ + Popover({ isOpen: true, position: "end-top", customStyles: { @@ -728,7 +728,8 @@ export const SubmenuInPopover = (context) => Popover({ "inline-size": "120px", }, content: [ - (args, context) => Template({ + Template({ + ...args, selectionMode: "single", items: [ { @@ -751,10 +752,8 @@ export const SubmenuInPopover = (context) => Popover({ label: "日本語", } ], - ...args, }, context) ], - ...args, }, context) ], }, context); From d948b2a1200d2bd3d18f13954ef5787b23e2ce6a Mon Sep 17 00:00:00 2001 From: Josh Winn <965114+jawinn@users.noreply.github.com> Date: Sat, 1 Mar 2025 10:15:46 -0500 Subject: [PATCH 10/11] docs(actionbutton): correct corner triangle sizing numbers --- components/actionbutton/stories/template.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/components/actionbutton/stories/template.js b/components/actionbutton/stories/template.js index 13d353d5306..3b3cb8930c2 100644 --- a/components/actionbutton/stories/template.js +++ b/components/actionbutton/stories/template.js @@ -113,8 +113,8 @@ export const Template = ({ xs: "75", s: "75", m: "100", - l: "400", - xl: "400", + l: "200", + xl: "300", }[size] || "100"), setName: "ui", customClasses: [`${rootClass}-hold`], From d8a1ef7074675eeb0e92f1a5cca2fb432c732105 Mon Sep 17 00:00:00 2001 From: Josh Winn <965114+jawinn@users.noreply.github.com> Date: Mon, 3 Mar 2025 10:03:07 -0500 Subject: [PATCH 11/11] docs(icon): add docs section about missing icon placeholder circle --- components/icon/stories/icon.mdx | 8 ++++++++ components/icon/stories/icon.stories.js | 17 +++++++++++++++++ 2 files changed, 25 insertions(+) diff --git a/components/icon/stories/icon.mdx b/components/icon/stories/icon.mdx index de5d3711be7..8aa57f0b338 100644 --- a/components/icon/stories/icon.mdx +++ b/components/icon/stories/icon.mdx @@ -67,6 +67,14 @@ with: +## Missing workflow icon placeholder + +In Storybook documentation, if a workflow icon name does not exist in the set, the placeholder "Circle" icon +will be shown. Missing ui icons will render nothing. The following example purposefully uses an +icon name that does not exist to demonstrate this behavior. + + + ## Repositories for the icon SVG files The UI icon SVGs are within the Spectrum CSS repository, which has its own package published to NPM: diff --git a/components/icon/stories/icon.stories.js b/components/icon/stories/icon.stories.js index d418250a69d..8fefeb451e1 100644 --- a/components/icon/stories/icon.stories.js +++ b/components/icon/stories/icon.stories.js @@ -193,3 +193,20 @@ UIArrows.tags = ["!dev"]; UIArrows.parameters = { chromatic: { disableSnapshot: true }, }; + +/** + * In Storybook documentation, if a workflow icon name does not exist in the set, the + * placeholder "Circle" icon will be shown. Missing ui icons will render + * nothing. The following example purposefully uses an icon name that does + * not exist to demonstrate this behavior. + */ +export const MissingWorkflowIcon = Default.bind({}); +MissingWorkflowIcon.storyName = "Missing workflow icon placeholder"; +MissingWorkflowIcon.tags = ["!dev"]; +MissingWorkflowIcon.args = { + setName: "workflow", + iconName: "ThisIconNameDoesNotExist", +}; +MissingWorkflowIcon.parameters = { + chromatic: { disableSnapshot: true }, +};