chore(calendar,card,coachmark): remove MDX files by marissahuysentruyt · Pull Request #3443 · adobe/spectrum-css

marissahuysentruyt · 2024-12-12T20:38:31Z

Description

This PR migrates any documentation from MDX files to the component's *.stories.js file. No loss of documentation should have occurred, but minor changes, like correcting story names to organize similar stories together or reorganizing content now that there is not an MDX file or fixing sentence-casing, is acceptable.

Components included are:

calendar (I updated the literal calendar year and updated the range selection)
card
- there seemed to be some instances of missing args, so those were added.
- I also beefed up some of our documentation and reorganized the stories.
coach mark (2 additional test cases were added, as well as with & without the action menu represented in the default story)

Jira/Specs

CSS-1049

How and where has this been tested?

Please tag yourself on the tests you've marked complete to confirm the tests have been run by someone other than the author.

Validation steps

Pull down the branch to run locally or visit the deploy preview.
Visit the calendar docs page.
Verify the information on the docs page matches the production docs page for calendar.
- Verify the updates to the calendar range selection are actually what happen in the DOM.
Repeat the process for the following components:
- card (here's the PR link to compare to card's production links: card's production)
- coach mark (and this PR link to compare to coach mark's production)

Additional Validation

Verify the calendar controls can be manipulated to create the stories on the docs page. The only story that couldn't be created was the range selection, so it is a story in the side nav now (which matches our convention in other components). More work on the template might help to achieve that, but it would be done in subsequent pull requests.
Verify the card controls can be manipulated to create the stories on the docs page. The only story that couldn't be created was the "no image," so it is a story in the side nav now (which matches our convention in other components). More work on the template might help to achieve that, but it would be done in subsequent pull requests.
Verify the 2 new test cases for coach (action menu + media and pagination + media) render when viewing the testing grid.

Regression testing

Validate:

The documentation pages for at least two other components are still loading, including:

The pages render correctly, are accessible, and are responsive.

If components have been modified, VRTs have been run on this branch:

VRTs have been run and looked at.
Any VRT changes have been accepted (by reviewer and/or PR author), or there are no changes.

Screenshots

To-do list

I have read the contribution guidelines.
I have updated relevant storybook stories and templates.
If my change impacts documentation, I have updated the documentation accordingly.
✨ This pull request is ready to merge. ✨

changeset-bot · 2024-12-12T20:38:37Z

⚠️ No Changeset found

Latest commit: 874f6cc

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

- adds some missing documentation regarding stories - reorganizes some information to sit with appropriate story/variant - migrates documentation from MDX file to the stories file instead

- migrates documentation from MDX file to the stories file instead

marissahuysentruyt · 2024-12-17T22:43:57Z

+			testHeading: "With action menu + media",
+			hasPagination: false,
+			isOpen: true,
+			hasImage: true,
+			wrapperStyles: {
+				"background-color": "var(--spectrum-gray-50, white)"
+			},
+		},
 		{
 			testHeading: "With pagination",
-			hasPagination: true,
 			hasActionMenu: false,
-			hasImage: false,
+			wrapperStyles: {
+				"background-color": "var(--spectrum-gray-50, white)"
+			},
+		},
+		{
+			testHeading: "With pagination + media",
+			hasActionMenu: false,


TODO: I have not run VRTs with these new tests yet.

marissahuysentruyt · 2024-12-17T22:46:47Z

-			title: "Card title",
-			description: "Optional description that should be one or two lines",
 			footer: undefined,
 			image: undefined,
 		},
 		{
 			testHeading: "Horizontal",
-			title: "Card title",
 			description: "jpg",
 			showAsset: "file",
-			isQuiet: false,


All the changes in here aren't necessary- we just don't need to redefine a bunch of stuff. This was sort of an extension of some of the removals in card.stories.js.

Great updates! Love an optimization. ❤️

marissahuysentruyt · 2024-12-17T22:47:48Z

                    {
                      size: "xxl",
-                      iconName: showAsset === "folder" ? "File" : "Document",
+                      iconName: showAsset === "folder" ? "Folder" : "Document",


"File" isn't an icon (apparently), but "Folder" is. Which actually makes more sense when `showAsset: "folder" 😆

Nice! Let's hope that translates to our S2 icons too when the time comes.

marissahuysentruyt · 2024-12-17T22:52:51Z

 			control: "date",
 			if: { arg: "isDisabled", truthy: false },
 		},
+		isRangeSelection : {


I could understand if we wanted to remove this for now. It definitely feels like we could get this to work in the template, but I didn't want to sink too much time dedicated to that, in this PR.

castastrophe

Super minor questions and suggestions. These updates are great! Thanks for not just migrating the mdx docs but also adding tests cases we were missing and clarifying the knobs in our stories!

castastrophe · 2024-12-18T15:00:28Z

+			name: "Days of the week",
+			description: "Use 3 letter abbreviation for day of week.",


Since this boolean is specifically to change the days of the week from spelled out to abbreviated, I wonder if this update is actually less clear? Perhaps the title should be something like, "Use abbreviated weekdays"?

castastrophe · 2024-12-18T15:01:26Z

-		isQuiet,
+		isQuiet: {
+			...isQuiet,
+			description: "`showAsset: 'image'` must be selected to properly render the quiet styles.",


If this is a requirement, should we update the controls with an if that requires it?

I took another look at this. I don't think showAsset is actually a requirement, so I'm handling the isQuiet arg differently in the template now. Let me know what you think! e2a3bb8

castastrophe · 2024-12-18T15:02:43Z

+			control: "boolean",
+			if: { arg: "isHorizontal", truthy: false },
+		},
+		isCardAssetOverride: {


Override feels a touch vague for what this is actually doing. Is there a more descriptive way to describe the change such as "Image fill" or "Gallery style"?

What do you think of isImageFill? Gallery already has a control, with different styles, so I went with isImageFill. That did require a change to args in asset, however. 874f6cc

castastrophe · 2024-12-18T15:03:43Z

-			title: "Card title",
-			description: "Optional description that should be one or two lines",
 			footer: undefined,
 			image: undefined,
 		},
 		{
 			testHeading: "Horizontal",
-			title: "Card title",
 			description: "jpg",
 			showAsset: "file",
-			isQuiet: false,


Great updates! Love an optimization. ❤️

castastrophe · 2024-12-18T15:04:14Z

                    {
                      size: "xxl",
-                      iconName: showAsset === "folder" ? "File" : "Document",
+                      iconName: showAsset === "folder" ? "Folder" : "Document",


Nice! Let's hope that translates to our S2 icons too when the time comes.

castastrophe · 2024-12-18T15:07:18Z

+	content: html`
+		${Container({
+			withBorder: false,
+			heading: "With action menu",
+			content: Template({...args, isOpen: true}, context),
+		})}
+		${Container({
+			withBorder: false,
+			heading: "Without action menu",
+			content: Template({...args, hasActionMenu: false}, context),
+		})}
+	`


This is perfect but just so you see a possible alternative, you can also pass content in using an array and then you don't have to add the html wrapper here:

Suggested change

content: html`

${Container({

withBorder: false,

heading: "With action menu",

content: Template({...args, isOpen: true}, context),

})}

${Container({

withBorder: false,

heading: "Without action menu",

content: Template({...args, hasActionMenu: false}, context),

})}

`

content: [

Container({

withBorder: false,

heading: "With action menu",

content: Template({...args, isOpen: true}, context),

}),

Container({

withBorder: false,

heading: "Without action menu",

content: Template({...args, hasActionMenu: false}, context),

})

]

* chore(card): remove default args from test cases * docs(card): remove MDX file - adds some missing documentation regarding stories - reorganizes some information to sit with appropriate story/variant - migrates documentation from MDX file to the stories file instead * docs(coachmark): remove MDX file - adds some missing documentation regarding stories - reorganizes some information to sit with appropriate story/variant - migrates documentation from MDX file to the stories file instead * chore(coachmark): adds extra chromatic coverage * docs(calendar): remove MDX file - migrates documentation from MDX file to the stories file instead * docs(calendar): clarifies days of the week control name * docs(card): clarify quiet control requirements * chore(coachmark): remove html wrapper in favor of content array * chore(card): fix isQuiet controls and usage in template * docs(asset,card): isCardAssetOverride renamed to isImageFill

marissahuysentruyt self-assigned this Dec 12, 2024

marissahuysentruyt added documentation Because documentation is important and shouldn't be broken size-1 XS ~1-6hrs; nearly trivial, a few hours, could do more than one in a single day. low priority Not a critical update or fix; can be deprioritized if necessary s1 labels Dec 12, 2024

marissahuysentruyt changed the title ~~Marissahuysentruyt/css 1048 refactor calendar card coachmark mdx~~ chore(calendar,card,coachmark): remove MDX files Dec 12, 2024

marissahuysentruyt added the wip This is a work in progress, don't judge. label Dec 12, 2024

castastrophe force-pushed the s2-foundations-redux branch 2 times, most recently from 2eebf01 to 711d4cd Compare December 16, 2024 23:49

marissahuysentruyt removed the wip This is a work in progress, don't judge. label Dec 17, 2024

marissahuysentruyt added 5 commits December 17, 2024 17:04

chore(card): remove default args from test cases

869b4a7

docs(card): remove MDX file

96c3a4a

- adds some missing documentation regarding stories - reorganizes some information to sit with appropriate story/variant - migrates documentation from MDX file to the stories file instead

docs(coachmark): remove MDX file

107f56f

- adds some missing documentation regarding stories - reorganizes some information to sit with appropriate story/variant - migrates documentation from MDX file to the stories file instead

chore(coachmark): adds extra chromatic coverage

2050b36

docs(calendar): remove MDX file

8feb55a

- migrates documentation from MDX file to the stories file instead

marissahuysentruyt force-pushed the marissahuysentruyt/css-1048-refactor-calendar-card-coachmark-mdx branch from 7f1d858 to 8feb55a Compare December 17, 2024 22:36

marissahuysentruyt marked this pull request as ready for review December 17, 2024 22:41

marissahuysentruyt added the ready-for-review label Dec 17, 2024

marissahuysentruyt commented Dec 17, 2024

View reviewed changes

marissahuysentruyt added skip_vrt Add to a PR to skip running VRT (but still pass the action) and removed skip_vrt Add to a PR to skip running VRT (but still pass the action) labels Dec 17, 2024

castastrophe suggested changes Dec 18, 2024

View reviewed changes

marissahuysentruyt added 4 commits December 18, 2024 11:00

docs(calendar): clarifies days of the week control name

e5960d2

docs(card): clarify quiet control requirements

3547bf7

chore(coachmark): remove html wrapper in favor of content array

eafa70e

chore(card): fix isQuiet controls and usage in template

e2a3bb8

docs(asset,card): isCardAssetOverride renamed to isImageFill

874f6cc

marissahuysentruyt requested a review from castastrophe December 18, 2024 18:18

castastrophe approved these changes Dec 19, 2024

View reviewed changes

castastrophe merged commit 049422a into s2-foundations-redux Dec 19, 2024

castastrophe deleted the marissahuysentruyt/css-1048-refactor-calendar-card-coachmark-mdx branch December 19, 2024 20:36

		name: "Days of the week",
		description: "Use 3 letter abbreviation for day of week.",

Conversation

marissahuysentruyt commented Dec 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Jira/Specs

How and where has this been tested?

Validation steps

Additional Validation

Regression testing

Screenshots

To-do list

Uh oh!

changeset-bot Bot commented Dec 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⚠️ No Changeset found

Uh oh!

marissahuysentruyt Dec 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

castastrophe left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

marissahuysentruyt commented Dec 12, 2024 •

edited

Loading

changeset-bot Bot commented Dec 12, 2024 •

edited

Loading

marissahuysentruyt Dec 17, 2024 •

edited

Loading