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

Create v6 Accordion with Styled System #391

Merged
merged 13 commits into from
Dec 30, 2020

Conversation

TyMick
Copy link
Collaborator

@TyMick TyMick commented Nov 27, 2020

As a step toward the v6 release (#378), how's this look for replacing the styleOverrides prop in Accordion?

The only property in styleOverrides used in Accordion was panelPadding, which set padding on all nested Accordion.Panels, so here are the main replacements:

  1. Added Styled System's variant API to Accordion.Panel to define the default and 'minimal' padding styles, then added the space field to allow overriding padding on individual panels.

    export const Panel = styled(Box).attrs(({ headerId, panelId }) => ({
      role: 'region',
      'aria-labelledby': `accordion-header-${headerId}`,
      id: `accordion-panel-${panelId}`,
    }))(
      variant({
        variants: {
          default: {
            padding: 6,
            paddingTop: 4,
          },
          minimal: {
            padding: 4,
            paddingTop: 0,
          },
        },
      }),
      space,
    );
  2. Added a custom panelPadding prop to Accordion, applying its effects only to nested Panels from Accordion.Panel. This way it's still possible to set the padding of all panels at the Accordion level instead of having to set each Accordion.Panel's padding separately.

    const AccordionBox = styled(Box)`
      ${Panel} {
        ${system({
          panelPadding: {
            property: 'padding',
            scale: 'space',
          },
        })}
      }
    `;

Should this PR target a new branch instead of master since it's a breaking change, or will v6 be the next release (so it doesn't matter)?

Replaces styleOverrides prop functionality with Styled System props.
@TyMick
Copy link
Collaborator Author

TyMick commented Nov 29, 2020

Come to think of it, should I be creating new files for these new v6 versions with breaking changes (this and #392 so far) instead of contemplating a new branch? I just reread the whole "December 18th - all updated components will be available at the /v6 import" part on #378.

@RobertBolender
Copy link
Contributor

Come to think of it, should I be creating new files for these new v6 versions with breaking changes (this and #392 so far) instead of contemplating a new branch? I just reread the whole "December 18th - all updated components will be available at the /v6 import" part on #378.

The pattern we've been using is to add a new file and export it at the /v6 entrypoint. While we will be making the v6 breaking change soon, we want to keep the v5 versions free of breaking changes until that point so that we're always in a shippable state.

Examples of recent v6 additions:
Popover
Dropdown

@TyMick TyMick marked this pull request as draft December 1, 2020 22:07
@TyMick TyMick marked this pull request as ready for review December 2, 2020 21:43
@TyMick
Copy link
Collaborator Author

TyMick commented Dec 2, 2020

I went with the "legacy" naming scheme, renaming old files to legacy-* and exporting the v5 version from components/accordion as LegacyAccordion.

How's this look?

@TyMick TyMick changed the title Replace styleOverrides prop in Accordion Create v6 Accordion with Styled System Dec 19, 2020
Accordion.Panel was the only child component that the old styleOverrides
prop affected, so that's why it's the only one I mentioned here.
@RobertBolender RobertBolender mentioned this pull request Dec 22, 2020
40 tasks
Robert Bolender added 4 commits December 30, 2020 14:12
The variant prop already has a default in the destructured props, and the expandedSections prop is required.
@RobertBolender RobertBolender merged commit 10c8d04 into Faithlife:master Dec 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants