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

Allow Styled tags to be updated via withComponent #232

Merged
merged 10 commits into from
Sep 25, 2017

Conversation

ifyoumakeit
Copy link
Contributor

@ifyoumakeit ifyoumakeit commented Aug 5, 2017

What:
Added withComponent method to allow styled components to update tag name. Components can also be extended via this method.

Why:
Tags may need to be changed to comply with accessibility and HTML standards. One use case is headings (h1, h2, etc...) that may need to be swapped depending on document depth.

The method na

How:
Added a new method to the Styled component that re-calls createStyled with the new tag/component.

Checklist:

  • Documentation
  • Tests
  • Code complete

@ifyoumakeit ifyoumakeit force-pushed the withComponent branch 5 times, most recently from bab4bf4 to 6bdf000 Compare August 8, 2017 18:33
@ifyoumakeit ifyoumakeit force-pushed the withComponent branch 3 times, most recently from 938bb62 to fd75382 Compare August 24, 2017 22:57
@ifyoumakeit
Copy link
Contributor Author

Fixing tests today.

@codecov
Copy link

codecov bot commented Sep 12, 2017

Codecov Report

Merging #232 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #232      +/-   ##
==========================================
+ Coverage   97.33%   97.34%   +0.01%     
==========================================
  Files          17       17              
  Lines         563      566       +3     
  Branches      134      134              
==========================================
+ Hits          548      551       +3     
  Misses         11       11              
  Partials        4        4
Impacted Files Coverage Δ
packages/react-emotion/src/index.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e4c5b29...735a364. Read the comment docs.

@ifyoumakeit ifyoumakeit force-pushed the withComponent branch 2 times, most recently from 29ee322 to ba9d250 Compare September 21, 2017 15:36
@ifyoumakeit
Copy link
Contributor Author

Running into some errors due to updates. Adding some documentation for sites too.

- Add `withComponent` method to create new styled component
- Update react test
- Update snapshot
@ifyoumakeit
Copy link
Contributor Author

ifyoumakeit commented Sep 22, 2017

Fixed up errors, but running into an issue where components created with withComponent lose their named css class. So css-Avatar-{xxxxx} css-{xxxxx} becomes css-{x} css-{xxxxx}

See Avatar class name here: https://deploy-preview-232--emotion.netlify.com/#withcomponent

Example.

<span data-reactroot="" class="css-Circle-wrfx8o0 css-mzfoqi">
  <img src="/a76dfa0d18a0536af9e917cdb8f873b9.png" class="css-1 css-mzfoqi">
</span>

@tkh44 / @mitchellhamilton , could this cause collisions? I was able to get it to use the same class as Circle css-Circle-wrfx8o0 but that seemed more confusing than helpful.

@emmatown
Copy link
Member

@ifyoumakeit This is because we create those unique classes with the identifier in the babel plugin. We could create a unique class for withComponent calls but that wouldn't work in the babel macro. We could make an export named withComponent or something instead of a method that accepts a styled component and a new tag which could work in the babel macro. I'm not really sure what we should do. @tkh44 what do you think?

@tkh44
Copy link
Member

tkh44 commented Sep 23, 2017

I need to think about this a bit more. We want to get this in version 8, but I want to do it right.

@ifyoumakeit
Copy link
Contributor Author

Makes sense, definitely want to make sure the solution is performant/useful and fits in well with the future of emotion.

@tkh44 tkh44 self-requested a review September 25, 2017 06:49
Copy link
Member

@tkh44 tkh44 left a comment

Choose a reason for hiding this comment

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

lgtm

docs/styled.md Outdated
@@ -66,3 +66,18 @@ const H2 = styled('h2')('some-other-class', {
})

```

### withComponent
Copy link
Member

Choose a reason for hiding this comment

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

I think we should make this its own file. Mention it and link to this new file. This way when we make docs pages withComponent will have its own page.

@emmatown emmatown merged commit d89fae5 into emotion-js:master Sep 25, 2017
@@ -90,6 +90,12 @@ export default function(tag, options: { e: string }) {
Styled.__emotion_styles = styles
Styled.__emotion_base = baseTag

Styled.withComponent = nextTag => {
return createStyled(nextTag, options)(styles)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 this is much cleaner and more obvious from first glance. Thanks @mitchellhamilton .

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.

3 participants