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

refactor: remove support for non-tw usage #92

Merged
merged 2 commits into from
Jan 21, 2021

Conversation

sastan
Copy link
Collaborator

@sastan sastan commented Jan 21, 2021

Currently apply, css, keyframes and animation support non-tw usage:

document.body.className = apply`bg-gray-50 text-gray-800`

const bounce = keyframes`...`
document.body.style.animation = `${bounce} 1s ease infinite`

This works as the returned function have toString and valueOf method which use the bound tw to generate the styles. Using a custom tw instance is supported by:

const { tw } = create(/* options */)

const kf = keyframes.bind(tw)

// Or providing tw on invocation
const bounce = keyframes.call(tw, {
  /* same as above */
})

This feature is quite difficult to understand for beginners and maybe confusing when stumbled on it in the docs. By removing it we can enforce that only explicit tw calls generate and inject styles. No magic!

I would like to drop this in favor of requiring the explicit use of tw or using a helper:

document.body.className = tw(apply`bg-gray-50 text-gray-800`)

// Using a helper
const twind = (...args) => tw(apply(...args))

document.body.className = twind`bg-blue bg-red`

I think that most use cases can be achieved now using a combination of tw with apply, css, keyframes and animation.

Would do you think? @tw-in-js/contributors

@coveralls
Copy link

coveralls commented Jan 21, 2021

Pull Request Test Coverage Report for Build 502073547

  • 18 of 21 (85.71%) changed or added relevant lines in 4 files are covered.
  • 7 unchanged lines in 3 files lost coverage.
  • Overall coverage decreased (-0.09%) to 90.04%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/twind/directive.ts 3 4 75.0%
src/css/index.ts 10 12 83.33%
Files with Coverage Reduction New Missed Lines %
src/twind/apply.ts 1 95.24%
src/css/index.ts 3 95.17%
src/twind/directive.ts 3 86.0%
Totals Coverage Status
Change from base Build 501295600: -0.09%
Covered Lines: 4224
Relevant Lines: 4728

💛 - Coveralls

@github-actions
Copy link
Contributor

Try the Preview Package

Official releases are only available on registry.npmjs.org as twind.

This PR has been published to npm.pkg.github.com as @tw-in-js/twind@pr92.

Install/Update

Configure your NPM client (click to expand)

Adjust you .npmrc to use https://npm.pkg.github.com for @tw-in-js

@tw-in-js:registry=https://npm.pkg.github.com

Using the command line:

npm config set @tw-in-js:registry https://npm.pkg.github.com --global
# For npm
npm install --force twind@npm:@tw-in-js/twind@pr92

# For yarn - upgrade implies install
yarn upgrade twind@npm:@tw-in-js/twind@pr92

@github-actions
Copy link
Contributor

github-actions bot commented Jan 21, 2021

size-limit report 📦

Path Size
dist/twind.js 12.01 KB (-0.36% 🔽)

@just214
Copy link
Collaborator

just214 commented Jan 21, 2021

My assumption would have been that these functions would only work within the context of a tw call, so this seems like a logical approach to me. I’ve never tried them outside of tw, so maybe I just don’t know what I’ve been missing. But, I really like the new changes and I like that you left the helper up to the consumer.

@sastan sastan merged commit f2b8ed5 into main Jan 21, 2021
@sastan sastan deleted the remove-implicit-to-string-conversion branch January 21, 2021 20:41
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.

None yet

3 participants