feat(illustratedmessage): adding new s2 tokens, horizontal layout, vrts #3246
There are no files selected for viewing
|
There was a problem hiding this comment. Can we make drop zone changes their own PR like Marissa suggested? Except for perhaps the renamed mods that are relevant to this PR. This will make it easier to review and validate. It is hard to validate drop zone changes before the component is fully migrated to S2. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -124,6 +124,11 @@ | |
|
|
||
| &.is-filled.is-dragged { | ||
| --mod-drop-zone-content-display: flex; | ||
| background-image: url("https://s3-alpha-sig.figma.com/img/18cd/d82f/a71c264919d2049dcb540ae23897d027?Expires=1733097600&Key-Pair-Id=APKAQ4GOSFWCVNEHN3O4&Signature=OMgIsMMWb10DyYC8c-pAUm4MxTwft~CTzqPojWvC0fLCFthNPTm5uh3esb1hTRxleJ9-HMNTvqawigP1GCbYI4PqCgkgMZA2CxvtksvQoi2qvVOBy-U6grTgw2KRFOkNEsT~UkcQRAdx0wDZs-PBGyeeMjs0jyZTmq7LbR5ntIVaFFNTpBTnj1o9Hl2S-Ofo1UEANW58Az~3JvY1XyuSYd3q1QIXb-lYi57QQ74F85P0B6ZDW~0n7o-9eg4SsJ0OgVgJu3LRr6XFyNCbb109Iu~wrmSHCjnCLm5~9v4lY0Tm8whnUYQ3js88LHFxUghP~3MJahtKgJXqMwJKNMhCCQ__"); | ||
|
There was a problem hiding this comment. Should we be linking to the asset in the repo? Could any of the dropzone CSS be left off of this PR, to save for the dropzone migration? My thought is that if you're doing the dropzone migration (which it looks like you are), let's separate as much as we can. I think I have some separate dropzone ideas that would be better suited in a separate PR. There was a problem hiding this comment. This definitely should link to an actual file and not the Figma; the link has expired and it is now "access denied". It looks like this URL may have been for the "placeholder-image" in the background of drop zone? If so, we shouldn't be providing any image in the CSS for the component as it looks like that is supposed to represent whatever image was currently selected. It's something we could show as an example in custom styles via the template, linking to a local asset, as part of the drop zone migration. |
||
|
|
||
| .spectrum-DropZone-actions { | ||
| margin-block-start: 0; | ||
| } | ||
jawinn marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| &:focus-visible { | ||
|
|
@@ -142,6 +147,10 @@ | |
| z-index: 10; | ||
| } | ||
|
|
||
| .spectrum-DropZone-actions { | ||
| margin-block-start: var(--spectrum-drop-zone-body-to-action); | ||
| } | ||
|
|
||
|
There was a problem hiding this comment. Does this |
||
| .spectrum-DropZone-button { | ||
| box-sizing: border-box; | ||
| border: none; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,7 +15,7 @@ governing permissions and limitations under the License. | |
| --spectrum-illustrated-message-heading-to-description: var(--spectrum-spacing-75); | ||
| --spectrum-illustrated-message-illustration-to-heading: var(--spectrum-spacing-200); | ||
| --spectrum-illustrated-message-illustration-to-content: var(--spectrum-spacing-200); | ||
| --spectrum-illustrated-message-description-to-action: var(--spectrum-spacing-300); | ||
| --mod-buttongroup-justify-content: center; | ||
rise-erpelding marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| /* Illustration */ | ||
|
|
@@ -24,15 +24,15 @@ governing permissions and limitations under the License. | |
| --spectrum-illustrated-message-illustration-size: 96px; | ||
|
|
||
| /* Title */ | ||
| --spectrum-illustrated-message-title-font-family: var(--spectrum-sans-font-family-stack); | ||
| --spectrum-illustrated-message-title-font-weight: var(--spectrum-title-sans-serif-font-weight); | ||
| --spectrum-illustrated-message-title-font-style: var(--spectrum-title-sans-serif-font-style); | ||
| --spectrum-illustrated-message-title-font-size: var(--spectrum-illustrated-message-medium-title-font-size); | ||
| --spectrum-illustrated-message-title-line-height: var(--spectrum-title-line-height); | ||
| --spectrum-illustrated-message-title-color: var(--spectrum-heading-color); | ||
|
|
||
| /* Description */ | ||
| --spectrum-illustrated-message-description-font-family: var(--spectrum-sans-font-family-stack); | ||
| --spectrum-illustrated-message-description-font-weight: var(--spectrum-body-sans-serif-font-weight); | ||
| --spectrum-illustrated-message-description-font-style: var(--spectrum-body-sans-serif-font-style); | ||
| --spectrum-illustrated-message-description-font-size: var(--spectrum-illustrated-message-medium-body-font-size); | ||
|
|
@@ -95,8 +95,9 @@ governing permissions and limitations under the License. | |
| color: var(--highcontrast-illustrated-message-illustration-color, var(--mod-illustrated-message-illustration-color, var(--spectrum-illustrated-message-illustration-color))); | ||
| fill: currentColor; | ||
| stroke: currentColor; | ||
| block-size: var(--mod-illustrated-message-illustration-block-size, var(--mod-illustrated-message-illustration-size, var(--spectrum-illustrated-message-illustration-size))); | ||
| inline-size: var(--mod-illustrated-message-illustrated-inline-size, var(--mod-illustrated-message-illustration-size, var(--spectrum-illustrated-message-illustration-size))); | ||
| grid-area: illustration; | ||
rise-erpelding marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| .spectrum-IllustratedMessage-accent { | ||
|
There was a problem hiding this comment. I don't see any references to the accent being removed in the changelog, but I also don't see any token specs for it. Might be a good thing to follow up with design on when this is being design reviewed. There was a problem hiding this comment. Checked in with design. Accent color is mainly used for the illustration and hover state in Drop zone. Illustrated message doesn't have accent color tokens There was a problem hiding this comment. Looking at the Figma specs for Drop Zone, it doesn't look like that accent on the illustration (by which I mean the little arrow that's blue where the rest of the illustration is gray) is in that either... we also don't use the accent color on drop zone now, the illustration is either all gray or all blue if it's dragged. It might be worth double-checking to see if that means the two-colored illustration is going away. If the illustrations in S2 are meant to always be a single color (whether blue or gray), we could probably take this Illustrated message (main, with accent): There was a problem hiding this comment. @rise-erpelding Two colored is going away and accent color is in Dropzone when dragged. |
||
|
|
@@ -133,8 +134,7 @@ governing permissions and limitations under the License. | |
|
|
||
| color: var(--mod-illustrated-message-description-color, var(--spectrum-illustrated-message-description-color)); | ||
|
|
||
| margin-block: 0; | ||
| } | ||
|
|
||
| .spectrum-IllustratedMessage--horizontal { | ||
|
|
@@ -143,8 +143,18 @@ governing permissions and limitations under the License. | |
| flex-direction: row; | ||
| text-align: start; | ||
| gap: var(--mod-illustrated-message-illustration-to-content, var(--spectrum-illustrated-message-illustration-to-content)); | ||
| display: grid; | ||
|
There was a problem hiding this comment. Toggling If using But if there's a clear reason to use grid for the horizontal variant, we can eliminate There was a problem hiding this comment. I was experimenting with There was a problem hiding this comment. @rise-erpelding I remember now - @marissahuysentruyt revealed the illustrations on the large size is out of bonds of block size. Using |
||
| grid-template-areas: "illustration body"; | ||
|
|
||
| .spectrum-IllustratedMessage-illustration { | ||
marissahuysentruyt marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| margin-block-end: 0; | ||
| } | ||
| } | ||
|
|
||
| .spectrum-IllustratedMessage--actions { | ||
| margin-block-start: var(--mod-illustrated-message-description-to-action, var(--spectrum-illustrated-message-description-to-action)); | ||
| } | ||
|
|
||
| .spectrum-IllustratedMessage--content { | ||
| grid-area: body; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,4 @@ | ||
| import { disableDefaultModes } from "@spectrum-css/preview/modes"; | ||
| import metadata from "../metadata/metadata.json"; | ||
| import packageJson from "../package.json"; | ||
| import { IllustratedMessageGroup } from "./illustratedmessage.test.js"; | ||
|
|
@@ -13,15 +11,6 @@ export default { | |
| title: "Illustrated message", | ||
| component: "IllustratedMessage", | ||
| argTypes: { | ||
| heading: { | ||
| name: "Heading", | ||
| type: { name: "string" }, | ||
|
|
@@ -33,10 +22,12 @@ export default { | |
| }, | ||
| description: { | ||
| name: "Description", | ||
| type: { name: "string" }, | ||
| table: { | ||
| type: { summary: "string" }, | ||
| category: "Content", | ||
| }, | ||
| control: "text", | ||
| }, | ||
| isHorizontal: { | ||
jawinn marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| name: "Horizontal orientation", | ||
|
|
@@ -73,7 +64,8 @@ export default { | |
| isHorizontal: false, | ||
| size: "m", | ||
| hasButtons: true, | ||
| heading: "Error 404: Page not found", | ||
| description: "This page isn't available. Try checking the URL or visit a different page.", | ||
| }, | ||
| parameters: { | ||
| design: { | ||
|
|
@@ -82,32 +74,22 @@ export default { | |
| }, | ||
| packageJson, | ||
| metadata, | ||
| layout: "centered" | ||
| }, | ||
| }; | ||
|
|
||
| export const Default = Template.bind({}); | ||
| Default.args = { | ||
| isHorizontal: false | ||
|
There was a problem hiding this comment. Since We could make it even clearer and add some documentation: |
||
| }; | ||
|
|
||
|
|
||
| export const Horizontal = Template.bind({}); | ||
|
There was a problem hiding this comment. Could we disable the chromatic snapshot for the horizontal story? I'm assuming once we add the horizontal story to the test file, we'll probably do that anyways! |
||
| Horizontal.tags = ["!dev"]; | ||
| Horizontal.args = { | ||
| isHorizontal: true | ||
| }; | ||
rise-erpelding marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| Horizontal.parameters = { | ||
| chromatic: { disableSnapshot: true }, | ||
| }; | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -10,22 +10,27 @@ export const Template = ({ | |||||||||||||||||||||||||||||||||||||||||||||||||
| heading, | ||||||||||||||||||||||||||||||||||||||||||||||||||
| description, | ||||||||||||||||||||||||||||||||||||||||||||||||||
| customClasses = [], | ||||||||||||||||||||||||||||||||||||||||||||||||||
| hasButtons = false, | ||||||||||||||||||||||||||||||||||||||||||||||||||
| isHorizontal = false, | ||||||||||||||||||||||||||||||||||||||||||||||||||
| useAccentColor = false, | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Do we need the If we can remove that class, we'll need to also remove that block Rise mentioned as well in the CSS.. |
||||||||||||||||||||||||||||||||||||||||||||||||||
| size = "m", | ||||||||||||||||||||||||||||||||||||||||||||||||||
| customIllustration, | ||||||||||||||||||||||||||||||||||||||||||||||||||
| }) => { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| const descriptionContent = Array.isArray(description) | ||||||||||||||||||||||||||||||||||||||||||||||||||
| ? description | ||||||||||||||||||||||||||||||||||||||||||||||||||
| : [() => html`${description}`]; | ||||||||||||||||||||||||||||||||||||||||||||||||||
| return html` | ||||||||||||||||||||||||||||||||||||||||||||||||||
| <div | ||||||||||||||||||||||||||||||||||||||||||||||||||
| class=${classMap({ | ||||||||||||||||||||||||||||||||||||||||||||||||||
| [rootClass]: true, | ||||||||||||||||||||||||||||||||||||||||||||||||||
| [`${rootClass}--horizontal`]: isHorizontal, | ||||||||||||||||||||||||||||||||||||||||||||||||||
| [`${rootClass}--size${size?.toUpperCase()}`]: typeof size !== "undefined" && size !== "m", | ||||||||||||||||||||||||||||||||||||||||||||||||||
| ...customClasses.reduce((a, c) => ({ ...a, [c]: true }), {}), | ||||||||||||||||||||||||||||||||||||||||||||||||||
| })} | ||||||||||||||||||||||||||||||||||||||||||||||||||
| > | ||||||||||||||||||||||||||||||||||||||||||||||||||
| ${customIllustration | ||||||||||||||||||||||||||||||||||||||||||||||||||
| ? customIllustration() | ||||||||||||||||||||||||||||||||||||||||||||||||||
| : illustrationSvgMarkup(size, useAccentColor)} | ||||||||||||||||||||||||||||||||||||||||||||||||||
| <div class="${rootClass}--content"> | ||||||||||||||||||||||||||||||||||||||||||||||||||
| ${when( | ||||||||||||||||||||||||||||||||||||||||||||||||||
| heading, | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -37,17 +42,20 @@ export const Template = ({ | |||||||||||||||||||||||||||||||||||||||||||||||||
| </h2>` | ||||||||||||||||||||||||||||||||||||||||||||||||||
| )} | ||||||||||||||||||||||||||||||||||||||||||||||||||
| ${when( | ||||||||||||||||||||||||||||||||||||||||||||||||||
| descriptionContent, | ||||||||||||||||||||||||||||||||||||||||||||||||||
| () => | ||||||||||||||||||||||||||||||||||||||||||||||||||
| html`<p | ||||||||||||||||||||||||||||||||||||||||||||||||||
| class="${rootClass}-description" | ||||||||||||||||||||||||||||||||||||||||||||||||||
| > | ||||||||||||||||||||||||||||||||||||||||||||||||||
| ${descriptionContent.map((c) => | ||||||||||||||||||||||||||||||||||||||||||||||||||
| typeof c === "function" ? c({}) : c | ||||||||||||||||||||||||||||||||||||||||||||||||||
| )} | ||||||||||||||||||||||||||||||||||||||||||||||||||
| </p>` | ||||||||||||||||||||||||||||||||||||||||||||||||||
| )} | ||||||||||||||||||||||||||||||||||||||||||||||||||
| ${when(hasButtons, () => | ||||||||||||||||||||||||||||||||||||||||||||||||||
| ButtonGroup({ | ||||||||||||||||||||||||||||||||||||||||||||||||||
| size, | ||||||||||||||||||||||||||||||||||||||||||||||||||
| customClasses: ["spectrum-IllustratedMessage--actions"], | ||||||||||||||||||||||||||||||||||||||||||||||||||
| items: [ | ||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| variant: "secondary", | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -66,77 +74,15 @@ export const Template = ({ | |||||||||||||||||||||||||||||||||||||||||||||||||
| </div> | ||||||||||||||||||||||||||||||||||||||||||||||||||
| </div> | ||||||||||||||||||||||||||||||||||||||||||||||||||
| `; | ||||||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| const illustrationSvgMarkup = (size = "m", withAccentColorClass = false) => { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| const computedSize = size === "l" ? 160 : 96; | ||||||||||||||||||||||||||||||||||||||||||||||||||
| return html` | ||||||||||||||||||||||||||||||||||||||||||||||||||
| <svg class="spectrum-IllustratedMessage-illustration" xmlns="http://www.w3.org/2000/svg" width=${computedSize} height=${computedSize} viewBox="0 0 160 160" fill="none" preserveAspectRatio="xMinYMid slice"> | ||||||||||||||||||||||||||||||||||||||||||||||||||
| <path className="${withAccentColorClass | ||||||||||||||||||||||||||||||||||||||||||||||||||
| ? "spectrum-IllustratedMessage-accent" | ||||||||||||||||||||||||||||||||||||||||||||||||||
| : ""}" fill-rule="evenodd" clip-rule="evenodd" d="M56.2825 50.793C61.8957 37.7312 74.5092 28.5 89.3299 28.5C108.539 28.5 124.014 43.962 125.409 63.2651C140.627 65.9995 152 79.7949 152 96.1349C152 114.396 137.783 129.5 119.883 129.5C119.853 129.5 119.825 129.5 119.8 129.499C119.775 129.5 119.751 129.5 119.726 129.5H31.8807C31.7163 129.5 31.5543 129.49 31.3951 129.471C18.254 129.016 8 117.815 8 104.385C8 94.2115 13.8616 85.3693 22.4022 81.4428C22.3022 80.4808 22.2483 79.4993 22.2483 78.4996C22.2483 62.9094 34.3921 49.9713 49.734 49.9713C51.9924 49.9713 54.1854 50.2579 56.2825 50.793ZM32.2594 121.5H119.543C119.611 121.497 119.679 121.496 119.747 121.496C119.774 121.496 119.799 121.496 119.821 121.497C119.848 121.497 119.87 121.498 119.889 121.498C119.904 121.499 119.919 121.499 119.931 121.5C133.067 121.473 144 110.292 144 96.1349C144 82.4618 133.788 71.5539 121.259 70.8142C119.114 70.6875 117.453 68.8895 117.495 66.7416C117.5 66.5158 117.504 66.4133 117.507 66.3391C117.51 66.2659 117.512 66.2201 117.512 66.1104C117.512 49.5915 104.732 36.5 89.3299 36.5C76.895 36.5 66.2004 45.0052 62.5117 57.0028C62.1777 58.0892 61.3973 58.9822 60.3655 59.4587C59.3337 59.9353 58.1478 59.9504 57.1042 59.5002C54.8242 58.5168 52.3404 57.9713 49.734 57.9713C39.1343 57.9713 30.2483 66.9966 30.2483 78.4996C30.2483 80.0856 30.4387 81.6435 30.7778 83.175C31.235 85.2398 30.0059 87.3038 27.9726 87.8855C21.1678 89.8321 16 96.3955 16 104.385C16 113.898 23.2726 121.333 31.9497 121.483C32.054 121.485 32.1573 121.49 32.2594 121.5ZM119.959 121.501L119.957 121.501C119.967 121.501 119.973 121.502 119.959 121.501Z" /> | ||||||||||||||||||||||||||||||||||||||||||||||||||
| </svg> | ||||||||||||||||||||||||||||||||||||||||||||||||||
| `; | ||||||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment.
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this should go in the
assetsfolder, instead of making a newdropzone/assetsfolder. What do you think? I couldn't think of another component that has its own assets directory, but I could be mistaken.