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

Update Caret with emotion #184

Closed
wants to merge 7 commits into from
Closed

Update Caret with emotion #184

wants to merge 7 commits into from

Conversation

emplums
Copy link

@emplums emplums commented Aug 3, 2018

This PR gives the system props to the Caret component.

I started making this PR, but once I was done I realized that maybe Caret doesn't need system props, since it's only supposed to be an internal component used by CaretBox? Thoughts?

@@ -24,15 +29,20 @@ exports[`Caret renders cardinal directions 1`] = `
<path
d="M-8,0L0,8L8,0"
fill="none"
stroke="#e1e4e8"
stroke="#000"
Copy link
Author

Choose a reason for hiding this comment

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

Not sure why this changed 🤔

src/Caret.js Outdated
@@ -120,4 +120,4 @@ Caret.propTypes = {
/* eslint-enable */
}

export default withDefaultTheme(Caret)
export default withSystemProps(withDefaultTheme(Caret))
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI: withSystemProps() calls withDefaultTheme() for us ✨

Copy link
Author

Choose a reason for hiding this comment

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

ahh ok!

Copy link
Author

Choose a reason for hiding this comment

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

Hey! So I removed withSystemProps and now it's just withDefaultTheme(Caret, COMMON) and the toImplementSystemProps test is failing 🤔

Copy link
Author

Choose a reason for hiding this comment

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

Oh wow I see what I did there 😂 Ignore me, I'm drinking coffee as we speak hahaha

@shawnbot shawnbot mentioned this pull request Aug 7, 2018
36 tasks
@emplums
Copy link
Author

emplums commented Aug 7, 2018

We decided it's silly to add system props to Caret since it should only be used in CaretBox, closing!

@emplums emplums closed this Aug 7, 2018
@shawnbot shawnbot deleted the update-caret branch August 7, 2018 19:51
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