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

fix(divider): add ability to style divider component with theme api #2478

Merged
merged 3 commits into from Nov 24, 2020
Merged

fix(divider): add ability to style divider component with theme api #2478

merged 3 commits into from Nov 24, 2020

Conversation

Zyclotrop-j
Copy link
Contributor

@Zyclotrop-j Zyclotrop-j commented Nov 13, 2020

Pull request checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes
    /start features)

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

Fixes: 2405

Divider component cannot be styled using the theme API.

What is the new behavior?

  • The divider component can be styled by the theme API
  • There are two divider variants, "solid" (default) and "dashed" (new) [Question: Keep dashed variant or delete together with stoy and test for variants?]
  • Move hardcoded opacity and borderColor to theme
  • Add tests for Divider (incl. this fix)
  • Add story to demo this bug-fix and its functionality

Example:

The following is now possible (compare with comment on issue 2405 ;) ):

const myCustomVarient = {
  borderColor: 'blue.500',
  opacity: 1,
}
.... add "myCustomVarient" to theme overwrites using the key "Divider".....
<Divider orientation="horizontal" varient="myCustomVarient " />

Does this introduce a breaking change?

  • Yes
  • No

Other information

This PR fixes only the Divider-theme-API styling bug, not the container-theme-API styling bug (both are grouped in 2405)!

@vercel
Copy link

vercel bot commented Nov 13, 2020

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/5c2gkoc7y
✅ Preview: https://chakra-ui-git-fix-divider-theming.chakra-ui.now.sh

@Zyclotrop-j
Copy link
Contributor Author

@with-heart , @ryanerras this addresses #2405

Someone familiar with the build might need to have a look at the build thought as yarn docs deploy exits with ERR! lerna Unknown argument: @chakra-ui/babel-plugin. (This PR doesn't touch the docs and - more generally - doesn't touch the build either, so I'm not sure what's going on there.)

@changeset-bot
Copy link

changeset-bot bot commented Nov 22, 2020

🦋 Changeset detected

Latest commit: 55524df

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

This PR includes changesets to release 11 packages
Name Type
@chakra-ui/layout Minor
@chakra-ui/theme Minor
@chakra-ui/react Patch
@chakra-ui/toast Patch
@chakra-ui/test-utils Patch
chakra-cra Patch
chakra-nextjs Patch
chakra-nextjs-ts Patch
@chakra-ui/props-docs Patch
@chakra-ui/docs 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

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 @Zyclotrop-j for this PR 💖
I added a comment regarding the versioning. The code looks good to me!

@@ -0,0 +1,6 @@
---
"@chakra-ui/layout": patch
"@chakra-ui/theme": patch
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use a minor bump for both packages, because this PR adds a feature. Adding something increments in semver terms the minor version.

"@chakra-ui/layout": minor
"@chakra-ui/theme": minor

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to "minor"

"@chakra-ui/theme": patch
---

fix(divider): add ability to style divider component with theme api
Copy link
Contributor

Choose a reason for hiding this comment

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


feat(divider): add ability to style divider component with theme 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.

Changed to "feat"

Rename the story from 'WidthStory' to 'DashedVariant' to clarify what it actually showcases
@Zyclotrop-j
Copy link
Contributor Author

@TimKolberger , I added/cahnged the requested changes - please -re-review and let me know if there's anything else you'd like/prefer to be changed. And thanks for reviewing!

@TimKolberger TimKolberger merged commit 9fdc61d into chakra-ui:develop Nov 24, 2020
@TimKolberger
Copy link
Contributor

@allcontributors please add @Zyclotrop-j for code

@allcontributors
Copy link
Contributor

@TimKolberger

I've put up a pull request to add @Zyclotrop-j! 🎉

@Zyclotrop-j Zyclotrop-j deleted the fix/divider-theming branch November 24, 2020 12:07
@github-actions github-actions bot mentioned this pull request Dec 3, 2020
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.

Ability to style Container and Divider components with the theme API (extendTheme) (v1)
2 participants