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 the type of Theme for StyledComponent #1708

Merged
merged 5 commits into from
Jan 9, 2020
Merged

Fix the type of Theme for StyledComponent #1708

merged 5 commits into from
Jan 9, 2020

Conversation

fabien0102
Copy link
Contributor

@fabien0102 fabien0102 commented Jan 8, 2020 •
edited

What:

Fix the type of Theme for styledComponent

Why:

#1707

How:

Trying to follow the previous resolution from #1632

Checklist:

  • Documentation N/A
  • Tests (I would love too but not sure how to add them since Theme must be globally override)
  • Code complete
  • Changeset N/A

@changeset-bot
Copy link

changeset-bot bot commented Jan 8, 2020 •
edited

🦋 Changeset is good to go

Latest commit: b9a5a59

We got this.

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

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jan 8, 2020 •
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 b9a5a59:

Sandbox Source
Emotion Configuration

@fabien0102
Copy link
Contributor Author

@Andarist Another quick question since I'm trying to make my project working with the emotion@next.

@emotion/serialized seams to not be up to date in npm (still consume Theme from @emotion/core instead of @emotion/react) can you publish a new version of @emotion/serialized to have the next version?

image

Andarist
Andarist reviewed Jan 9, 2020
View reviewed changes
packages/styled/types/tests.tsx Outdated
@@ -86,6 +86,7 @@ const Button = styled.button``
const Input = styled.input`
& + ${Label}: {
margin-left: 3px;
color: ${props => props.theme.primary};
Copy link
Member

Choose a reason for hiding this comment

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

Have you confirmed that this would be broken without your fix?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep! definetely broken before and fix after 😉

Andarist
Andarist reviewed Jan 9, 2020
View reviewed changes
packages/styled/types/tests.tsx Outdated
@@ -86,6 +86,7 @@ const Button = styled.button``
const Input = styled.input`
Copy link
Member

Choose a reason for hiding this comment

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

could you also add one a little bit simpler test that would test this thing alone? this one is testing that Label can be interpolated as a part of the selector

@Andarist
Copy link
Member

Andarist commented Jan 9, 2020

@emotion/serialized seams to not be up to date in npm (still consume Theme from @emotion/core instead of @emotion/react) can you publish a new version of @emotion/serialized to have the next version?

Interesting, this might be a slight problem with our defined dependencies. Gonna discuss with @mitchellhamilton and release a fix for this later.

@Andarist
Copy link
Member

Andarist commented Jan 9, 2020

I have prepared a fix for @emotion/serialize: #1710

Could you add a changeset to this PR?

@fabien0102
Copy link
Contributor Author

fabien0102 commented Jan 9, 2020 •
edited

I have prepared a fix for @emotion/serialize: #1710

Could you add a changeset to this PR?

image

Something like this? (sorry, first time, so I prefer check ^^)

Edit: Regarding your other PR, I guess emotion/styled should be enough

@Andarist
Copy link
Member

Andarist commented Jan 9, 2020

Yes - exactly. You should only list @emotion/styled in it though (@emotion/serialize problem is going to be handled through other PR) and write a little bit more descriptive message about the fix: what was broken and how it got fixed (not necessarily with code examples, just please provide more context about the fix). Not sure if you have used it but you can run yarn changeset to get a nice CLI experience for creating a changeset (you don't have to do that now, as you already have a file created and you can just adjust it manually).

@fabien0102
Copy link
Contributor Author

I used yarn changeset, was very nice 👌 I may stole this tool for some of my projects 😁

@Andarist
Copy link
Member

Andarist commented Jan 9, 2020

I used yarn changeset, was very nice 👌 I may stole this tool for some of my projects 😁

👌 highly recommend it, much better than semantic commits in my opinion. Great especially for monorepos (it manages version bumps between workspace packages), but can be also used for single-package repositories as well.

@fabien0102 @Andarist
Better changeset
b9a5a59 
Co-Authored-By: Mateusz Burzyński <mateuszburzynski@gmail.com>
@Andarist Andarist merged commit b79781f into emotion-js:next Jan 9, 2020
@Andarist
Copy link
Member

Andarist commented Jan 9, 2020

Thanks ❤️

@Andarist Andarist mentioned this pull request Jan 9, 2020
Closed
louisgv pushed a commit to louisgv/emotion that referenced this pull request Sep 6, 2020
@fabien0102 @Andarist
Fix the type of props.theme for StyledComponent without AdditionalP…
921beea 
…rops (emotion-js#1708)

* Fix Theme type

* Add unit test for theme type

* Extract the Theme test in a separate test

* Add changeset

* Better changeset

Co-Authored-By: Mateusz Burzyński <mateuszburzynski@gmail.com>

Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

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

None yet
2 participants
@fabien0102 @Andarist