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

Conversation

booc0mtaco
Copy link
Contributor

@booc0mtaco booc0mtaco commented Jul 22, 2024

  • add in new props
  • update tests
  • add in warnings and assertions
  • export new components for checkbox and radio button groups
Screenshot 2024-07-22 at 19 13 13 Screenshot 2024-07-24 at 17 30 47 Screenshot 2024-07-24 at 17 30 51 Screenshot 2024-07-24 at 17 30 58 Screenshot 2024-07-24 at 17 31 02 Screenshot 2024-07-24 at 17 31 07

Test Plan:

  • Wrote automated tests
  • CI tests / new tests are not applicable
  • Manually tested my changes, but I want to keep the details secret
  • Created and used an alpha publish
  • Manually tested my changes, and here are the details:

Copy link

codecov bot commented Jul 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.43%. Comparing base (cc99f7e) to head (2704159).

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #2023      +/-   ##
==========================================
+ Coverage   97.40%   97.43%   +0.02%     
==========================================
  Files         109      109              
  Lines        2582     2608      +26     
  Branches      648      665      +17     
==========================================
+ Hits         2515     2541      +26     
  Misses         65       65              
  Partials        2        2              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

github-actions bot commented Jul 22, 2024

size-limit report 📦

Path Size
components 89.26 KB (+0.44% 🔺)
styles 27.71 KB (-0.27% 🔽)

@booc0mtaco booc0mtaco force-pushed the aholloway/EDS-1242_EDS-1237 branch 3 times, most recently from 4211930 to c88a967 Compare July 24, 2024 21:46
@@ -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

@@ -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)

@@ -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

@@ -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

@@ -180,7 +180,7 @@
border-color: var(--eds-theme-color-border-utility-critical);
}

& ~ .radio__selected {
&:checked ~ .radio__selected {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixes bug where selected treatment is applied to ALL radio buttons regardless of checked state

- add in new props
- update tests
- add in warnings and assertions (with tests)
- add in implementation examples for using radio buttons and checkboxes
@booc0mtaco booc0mtaco marked this pull request as ready for review July 24, 2024 22:28
@booc0mtaco booc0mtaco requested a review from a team July 24, 2024 22:30

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)

@booc0mtaco booc0mtaco merged commit e542b32 into next Jul 25, 2024
10 checks passed
@booc0mtaco booc0mtaco deleted the aholloway/EDS-1242_EDS-1237 branch July 25, 2024 12:11
@booc0mtaco booc0mtaco mentioned this pull request Jul 25, 2024
booc0mtaco added a commit that referenced this pull request Jul 26, 2024
## [15.2.0](v15.1.0...v15.2.0) (2024-07-25)

[Storybook](https://61313967cde49b003ae2a860-bksjqddurj.chromatic.com/)

### Features

* **Fieldset:** update styles to support new 2.0 components ([#2023](#2023)) ([e542b32](e542b32))


### Bug Fixes

* **Button:** update typography tokens to match design ([#2019](#2019)) ([3b94114](3b94114))
* **InputField:** align text content with inputWithin spacing ([#2020](#2020)) ([e87767b](e87767b))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants