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.

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

feat(Fieldset): update styles to support new 2.0 components #2023

Merged
merged 1 commit into from
Jul 25, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 2 additions & 1 deletion plop-templates/Component/Component.test.ts.hbs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { generateSnapshots } from '@chanzuckerberg/story-utils';
import * as stories from './{{pascalCase name}}.stories';
import type { StoryFile } from '../../util/utility-types';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Improve: Update the plop template to do the right things


describe('<{{pascalCase name}} />', () => {
generateSnapshots(stories);
generateSnapshots(stories as StoryFile);
Copy link
Contributor

Choose a reason for hiding this comment

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

Dang, this is annoying 😞. Shouldn't be necessary, but it's not obvious to me what the deal is.

What's weird is this works without casting in Bento. That project is using Storybook 8.2.4. Maybe there was a type regression in 8.2.5?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This started happening sorta randomly when I first upgraded to storybook 7. Not every file threw errors at first, and I also couldn't figure out what made some imports freak out like this ... 🤔 Looks like all the tests now have this cast.

Removing the cast yields this TS error:

Argument of type 'typeof import("/**/$Component.stories")' is not assignable to parameter of type 'StoryFile'.
  The types of 'default.play' are incompatible between these types.
    Type 'PlayFunction<ReactRenderer, AccordionProps> | undefined' is not assignable to type 'PlayFunction<ReactRenderer, Args> | undefined'.
      Type 'PlayFunction<ReactRenderer, AccordionProps>' is not assignable to type 'PlayFunction<ReactRenderer, Args>'.
        Types of parameters 'context' and 'context' are incompatible.
          Type 'PlayFunctionContext<ReactRenderer, Args>' is missing the following properties from type 'PlayFunctionContext<ReactRenderer, AccordionProps>': context, canvas, mount ts(2345)

});
5 changes: 5 additions & 0 deletions plop-templates/Component/Component.tsx.hbs
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,13 @@ import React from 'react';
import styles from './{{pascalCase name}}.module.css';

export interface Props {
// Component API
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Improve: Add newer templating where we group the props by those needed by eng (this area), from those to match what's in figma (Design API)

/**
* CSS class names that can be appended to the component.
*/
className?: string;
// Design API
// Insert props/values as defined in figma for {{pascalCase name}}
}

/**
Expand All @@ -18,13 +21,15 @@ export interface Props {
*/
export const {{pascalCase name}} = ({
className,
// Add other deferenced props to use
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Make sure the plopped file generates code that isn't immediately invalid to prettier but also a few more instruction areas

...other
}: Props) => {
const componentClassName = clsx(styles['{{dashCase name}}'], className);

return (
<div
className={componentClassName}
// Other de-referenced props go here
{...other}
>
Hello!
Expand Down
32 changes: 1 addition & 31 deletions plopfile.js
Original file line number Diff line number Diff line change
Expand Up @@ -52,37 +52,7 @@ module.exports = (plop) => {
template:
"export { default as {{pascalCase name}} } from './components/{{pascalCase name}}';",
},
// From https://github.com/bradfrost/czi-vanilla-storybook
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this sorting removes the useful comments and sections in the index.ts file. Updated the comment so if we want to restore the sorting, can look at the diff associated with the comment

function sortIndex() {
process.chdir(plop.getPlopfilePath());

const fs = require('fs');
const indexFile = `${plop.getDestBasePath()}/src/index.ts`;

if (fs.existsSync(indexFile)) {
// Split the index file into lines.
const lines = fs.readFileSync(indexFile, 'utf8').split('\n');
// Only sort lines that begin with "export {" in order to ignore
// comments and exported types.
const sortableLines = lines.filter((line) =>
/^export {.*/g.test(line),
);
// Ignore all other lines and turn them back into a string.
const ignoreLines = lines
.filter((line) => !/^export {.*/g.test(line))
.join('\n');

// Sort the sortable lines.
const sortedLines = sortableLines
.sort((a, b) => a.localeCompare(b))
.join('\n');

// Write all of the lines back to the file.
fs.writeFileSync(indexFile, `${sortedLines}\n${ignoreLines}\n`);

return `index.ts lines sorted`;
}
},
// Removed sort to avoid removing per-version component ordering and documentation (see diff for old code)
],
});
};
40 changes: 14 additions & 26 deletions src/components/Fieldset/Fieldset.module.css
Original file line number Diff line number Diff line change
@@ -1,50 +1,38 @@
@import '../../design-tokens/mixins.css';
.fieldset__footer {
display: flex;
justify-content: space-between;

/*------------------------------------*\
    # FIELDSET
\*------------------------------------*/

/**
* The fieldset groups a legend and several controls.
*/
.fieldset {
@mixin fieldsetStyles;
/* Overrides default browser styling. */
margin: calc(var(--eds-size-2) / 16 * 1rem);
margin-top: calc(var(--eds-size-3) / 16 * 1rem);
}

/*------------------------------------*\
# FIELDSET ITEMS
\*------------------------------------*/

/**
* The contents of the fieldset. Spaces them apart.
*/
.fieldset-items > :not(:last-child) {
margin-bottom: calc(var(--eds-size-1-and-half) / 16 * 1rem);
margin-bottom: calc(var(--eds-size-2) / 16 * 1rem);
}

/*------------------------------------*\
# FIELDSET LEGEND
\*------------------------------------*/

/**
* A label that's rendered as a <legend> for fieldsets.
* It contains the same characteristics as a label (ability to add flag for optional field, etc),
* but semantically/stylistically a bit different.
*/
.fieldset-legend {
/* Removes some default browser styles. */
@mixin legendReset;
font: var(--eds-theme-typography-form-label);
/* Adjust margin between legend and option list */
margin-bottom: calc(var(--eds-size-2) / 16 * 1rem);
margin-bottom: calc(var(--eds-size-3) / 16 * 1rem);
}

/**
* Label flag to mark whether or not a field is required
* or optional. Currently a flag is only present for optional fields
*
* TODO: remove as deprecated with v16 of EDS
*/
.fieldset-legend__flag {
font: var(--eds-theme-typography-body-sm);
}

.fieldset-legend__overline {
display: flex;
align-items: center;
gap: calc(var(--eds-size-half) / 16 * 1rem);
}
Loading
Loading