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

Remove theme from new APIs #974

Closed
wants to merge 5 commits into from
Closed

Conversation

emmatown
Copy link
Member

@emmatown emmatown commented Oct 29, 2018

What:
Remove passing the theme to the css prop, Global and ClassNames

Why:
Because of #973, even if we release v10 before hooks are available, I don't think we should introduce an API and then remove it soon after, especially since the usage of this would be spread throughout a codebase and practically impossible to codemod.

How:

Checklist:

  • Documentation
  • Tests
  • Code complete

@codecov
Copy link

codecov bot commented Oct 29, 2018

Codecov Report

Merging #974 into master will decrease coverage by 0.01%.
The diff coverage is 82.75%.

Impacted Files Coverage Δ
packages/core/src/jsx.js 97.77% <100%> (-0.23%) ⬇️
packages/core/src/global.js 97.14% <100%> (-0.36%) ⬇️
packages/core/src/class-names.js 71.69% <80.76%> (-0.53%) ⬇️

@Andarist
Copy link
Member

Not long time ago you've thought that exposing theme in those places were a good idea. What has changed? Just ease of how theme can be accessed? Was it really harder before?

I realize that this question might sound a little bit harsh (not being native english speaker I'm not always sure how to express my intentions clearly), but that's just an honest question - I'm literally curious.

@emmatown
Copy link
Member Author

Not long time ago you've thought that exposing theme in those places were a good idea. What has changed? Just ease of how theme can be accessed? Was it really harder before?

Mainly how it can be accessed. Having it accessible without having to add a render prop or HOC is so much more convenient. I've thought the current theming API had many problems(personally, my biggest problem with it is type safety) for a while but I wasn't sure how to adequately solve it and hooks along with context solve it perfectly so I don't want to add an API and then soon after deprecate. I thought adding the theme to these places was a good idea because the tools to build a better theming API didn't exist, but now they do so I want to prepare for it.

I realize that this question might sound a little bit harsh (not being native english speaker I'm not always sure how to express my intentions clearly), but that's just an honest question - I'm literally curious.

No worries!!! Please be critical!! I want to make emotion as good as it can possibly be and having criticism is important for that!

@emmatown
Copy link
Member Author

I've been thinking about this more and talked to @tkh44 and we've decided to keep this at the absolute minimum for v10(likely for a couple major versions) since it's kind of strange to remove an api when a replacement isn't available yet and the amount of code it requires is small and will get even smaller when hooks are ready

@emmatown emmatown closed this Nov 14, 2018
@Andarist Andarist deleted the remove-theme-from-new-apis branch November 14, 2018 17:52
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