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.

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

Jump to bottom

fix(accordion): introduce styles.root for Accordion #5595

Merged
merged 1 commit into from Feb 17, 2022
Merged

fix(accordion): introduce styles.root for Accordion #5595

merged 1 commit into from Feb 17, 2022

Conversation

takethefake
Copy link
Contributor

@takethefake takethefake commented Feb 16, 2022 •
edited

closes #5593

📝 Description

Updated style config for Accordion

This change introduces styles.root for Accordion

💣 Is this a breaking change (Yes/No):

No

@changeset-bot
Copy link

changeset-bot bot commented Feb 16, 2022 •
edited

🦋 Changeset detected

Latest commit: 2e2b947

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 6 packages
Name Type
@chakra-ui/accordion Patch
@chakra-ui/theme Patch
@chakra-ui/react Patch
@chakra-ui/toast Patch
@chakra-ui/props-docs Patch
@chakra-ui/test-utils Patch

Not sure what this means? Click here to learn what changesets are.

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

@vercel
Copy link

vercel bot commented Feb 16, 2022 •
edited

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/chakra-ui/chakra-ui-storybook/GzB8Urp8YShYv3tHRWfHggB26YRM
✅ Preview: https://chakra-ui-storybook-git-fork-takethefake-fix-5-6d4636-chakra-ui.vercel.app

@codesandbox-ci
Copy link

codesandbox-ci bot commented Feb 16, 2022 •
edited

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 2e2b947:

Sandbox Source
create-react-app-ts Configuration
sweet-perlman-blff7u Issue #5593

@TimKolberger
Copy link
Contributor

Thank you @takethefake for your time creating this PR!

I think there might be a way to fix this issue without a breaking change.

What do you think about adding a new part root to the accordionAnatomy and using it instead of moving container from the AccordionItem to the Accordion?

With v2 in mind we could reuse root for every MultipartComponent to make it a pattern. Why root and not container? Some components might have a container, like Tab or Accordion, but when thinking about a button it is hard to define where the container is. root is IMO easy to identify as the style root, which is always the top most DOM element of a component.

Kindly let me know your thoughts!

@TimKolberger TimKolberger added the Breaking Change 🔨 Changes that break compatibility with previous versions of Chakra UI. label Feb 17, 2022
@takethefake takethefake force-pushed the fix/5593-accordion-theme-styles branch from cf6561e to 15a03cb Compare February 17, 2022 07:31
@takethefake
Copy link
Contributor Author

Hej @TimKolberger

this sounds like a good approach to provide new functionality. We definitely need to make sure in v2 that we unify the usage of root / container to be clearly.

Adjusted the pr accordingly.

@takethefake takethefake changed the title fix(accordion): introduce styles.item for AccordionItem and move styles.container to Accordion fix(accordion): introduce styles.root for Accordion Feb 17, 2022
TimKolberger
TimKolberger suggested changes Feb 17, 2022
View reviewed changes
Copy link
Contributor

@TimKolberger TimKolberger left a comment

Choose a reason for hiding this comment

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

Thank you for updating the PR so fast, I added a few comments.
Then it looks good to go 🚀

packages/anatomy/src/index.ts Show resolved Hide resolved
packages/accordion/src/accordion.tsx Outdated
@@ -58,6 +58,10 @@ export const Accordion = forwardRef<AccordionProps, "div">(
[context, reduceMotion],
)

const rootStyles: SystemStyleObject = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this be inlined to the __css prop?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure!

Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome 😊

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@TimKolberger adjusted it

@takethefake takethefake force-pushed the fix/5593-accordion-theme-styles branch from 15a03cb to 1bd0d76 Compare February 17, 2022 08:42
@takethefake takethefake removed the Breaking Change 🔨 Changes that break compatibility with previous versions of Chakra UI. label Feb 17, 2022
@takethefake takethefake force-pushed the fix/5593-accordion-theme-styles branch from 1bd0d76 to 48dc7ff Compare February 17, 2022 08:46
@takethefake takethefake force-pushed the fix/5593-accordion-theme-styles branch from 48dc7ff to 2e2b947 Compare February 17, 2022 11:40
TimKolberger
TimKolberger approved these changes Feb 17, 2022
View reviewed changes
Copy link
Contributor

@TimKolberger TimKolberger left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@TimKolberger TimKolberger merged commit 0542b8a into chakra-ui:main Feb 17, 2022
@github-actions github-actions bot mentioned this pull request Feb 17, 2022
Merged
@yicru yicru mentioned this pull request Nov 15, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TimKolberger TimKolberger TimKolberger approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Accordion items not stylable as a theme part (or at least not how you'd expect it to)
2 participants
@takethefake @TimKolberger