Skip to content

chore: spectrum-two fixes #3514

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

Merged
merged 3 commits into from
Jan 22, 2025
Merged

chore: spectrum-two fixes #3514

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
72dd631
docs(tooltip): add missing args to template
marissahuysentruyt Jan 21, 2025
fee9e00
chore(picker): add/correct arg naming
marissahuysentruyt Jan 21, 2025
392cb52
chore(picker): remove window.isChromatic calls and click event
marissahuysentruyt Jan 21, 2025
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
2 changes: 1 addition & 1 deletion components/picker/stories/picker.stories.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ export default {
},
isHovered,
isActive,
content: { table: { disable: true } },
popoverContent: { table: { disable: true } },
Copy link
Collaborator Author

@marissahuysentruyt marissahuysentruyt Jan 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

During the picker docs-to-storybook migration, we tried to clarify this picker control by calling it popoverContent. This is the content that gets passed to the popover component in the template.

Popover has its own content arg, so that's why you'll see the content: popoverContent assignment.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Separately, there might be more that we can pull from the docs migration PR listed above. It looks like some of those additional controls/args (like currentValue, isHovered, showWorkflowIcon, and maybe some others) are missing currently. That could probably be figured out in a separate PR.

},
args: {
rootClass: "spectrum-Picker",
Expand Down
19 changes: 7 additions & 12 deletions components/picker/stories/template.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,8 +64,7 @@ export const Picker = ({
id=${id}
style=${styleMap(customStyles)}
type="button"
@click=${() => {
if (window.isChromatic()) return;
@click=${function() {
updateArgs({ isOpen: !isOpen });
}}
Comment on lines -68 to 69
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

On main, this line is @click=${onclick}, and the onclick function was defined in the Popover call. This template is pretty different from main, so I plopped the function here for now.

Any thoughts or reservations on this?

Copy link
Member

Choose a reason for hiding this comment

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

Just a nit, but should we keep it formatted as an arrow function? ✨

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure I know enough about why this part of the code is different between the branches/how Chromatic normally handles Picker, but it does work as is, and would work as an arrow function as well 🤷‍♀️

aria-labelledby=${ifDefined(ariaLabeledBy)}
Expand Down Expand Up @@ -109,6 +108,7 @@ export const Template = ({
size = "m",
label,
labelPosition = "top",
placeholder,
helpText,
isQuiet = false,
isOpen = false,
Expand All @@ -125,7 +125,7 @@ export const Template = ({
// Helps ensure that Popover appears below the Picker, with side labels layout.
display: "block",
},
content = [],
popoverContent = [],
} = {}, context = {}) => {
const pickerMarkup = Picker({
size,
Expand All @@ -134,17 +134,18 @@ export const Template = ({
isInvalid,
isDisabled,
isLoading,
content,
placeholder,
popoverContent,
labelPosition,
ariaLabeledBy: fieldLabelId,
}, context);

const popoverMarkup = content.length !== 0 ? Popover({
const popoverMarkup = popoverContent.length !== 0 ? Popover({
isOpen: isOpen && !isDisabled && !isLoading,
withTip: false,
position: "bottom",
isQuiet,
content,
content: popoverContent,
size,
customStyles: customPopoverStyles,
}, context) : "";
Expand Down Expand Up @@ -193,12 +194,6 @@ export const Template = ({
}, context)
)}
</div>`;

// Make sure there is a wrapper around sibling components when using the Chromatic
// template, so their layout is not affected by the flex and grid layouts used.
if (window.isChromatic()) {
return html`<div style="position: relative;">${markup}</div>`;
}
return markup;
};

Expand Down
1 change: 1 addition & 0 deletions components/tooltip/stories/template.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ export const Template = ({
rootClass = "spectrum-Tooltip",
label,
placement,
variant = "neutral",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Looks like the informative & negative tooltips got added back to Figma 11/18/24 (approximately). We'll have to create a card to implement the CSS styles and stories (for the docs page & controls)

variant: {
			name: "Variant",
			type: { name: "string" },
			table: {
				type: { summary: "string" },
				category: "Component",
			},
			options: ["neutral", "info", "negative"],
			control: "inline-radio",

tooltip migration PR: https://github.com/adobe/spectrum-css/pull/2743/files#diff-14d4b487bc2de8da9dccaef67bce6cccc92cbce28e2ac16912884edbce436cbc

Copy link
Collaborator

Choose a reason for hiding this comment

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

🙀 Good catch! Who knew those variants would come back again?

isOpen = true,
isFocused = false,
showOnHover = false,
Expand Down
Loading