Skip to content
This repository has been archived by the owner on Aug 19, 2022. It is now read-only.

Handle multiple animationName props #909

Conversation

jaredbrookswhite
Copy link
Contributor

Method to handle #675.

@coveralls
Copy link

coveralls commented Jul 13, 2017

Coverage Status

Coverage increased (+0.04%) to 94.016% when pulling f4a7e7d on jaredbrookswhite:feature-multiple-animationName-props into 42dba09 on FormidableLabs:master.

@kylecesmat
Copy link
Contributor

kylecesmat commented Aug 22, 2018

@jaredbrookswhite could you refresh this PR? We'd like to get it merged :)

looks like it's failing on a prettier task.

/home/travis/build/FormidableLabs/radium/examples/app.jsx
  84:19  error  Follow `prettier` formatting (expected '\n' but found 'm')  prettier/prettier
✖ 1 problem (1 error, 0 warnings)

Copy link
Contributor

@kylecesmat kylecesmat left a comment

Choose a reason for hiding this comment

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

Thank you so much for the great test coverages! This PR looks great, just a few pieces of feedback

if (
key === 'animationName' &&
value &&
(value.__radiumKeyframes || Array.isArray(value))
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we do this check above the if block?

let value = style[key];
const isKeyframeArray = Array.isArray(value);

if (
  key === 'animationName' &&
  value &&
  (value.__radiumKeyframes || isKeyframeArray)
) {
  if (isKeyframeArray) {
   ...etc
  }

config.userAgent,
);
addCSS(css);
value = animationName;
Copy link
Contributor

Choose a reason for hiding this comment

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

can we reduce the duplicate logic here and extract this to a shared method?

ie:

const processKeyframeStyle = (value) => {
  const keyframesValue = (value: Keyframes);
  const { animationName, css } = keyframesValue.__process(config.userAgent);
  addCSS(css);
  return animationName;
}
if (Array.isArray(value)) {
  value = value.map(processKeyframeStyle).join(', ');
} else {
  value = processKeyframeStyle(value);
}

@jaredbrookswhite jaredbrookswhite force-pushed the feature-multiple-animationName-props branch from e8630bd to 0839fe5 Compare August 22, 2018 16:33
@kylecesmat kylecesmat dismissed their stale review August 22, 2018 16:34

Allow appveyor to run

@jaredbrookswhite jaredbrookswhite force-pushed the feature-multiple-animationName-props branch from 0839fe5 to 88a1e38 Compare August 22, 2018 16:34
@kylecesmat
Copy link
Contributor

I believe you need to rebase/merge the upstream master branch. Looks like there is a merge conflict with the remote app.jsx

jaredbrookswhite and others added 19 commits August 22, 2018 11:38
* Switch to `publishr` on npm registry publication. Fixes FormidableLabs#894
* Move `rimraf` to `devDependencies` on publish.
* Rename + harmonize some npm scripts names.
* Supersedes and closes FormidableLabs#930 
* Fixes FormidableLabs#928
* Actually lint our root JS config files.
* Upgrade dev deps for React 16
* Fix tests. (And yowza there's some wonkiness with exception throwing...)
* Supersedes FormidableLabs#937 
* Updates `React.createClass` calls to real ES `class`.
* Refactor remaining `.jsx` suffixes to `.js` + configs.
* Actually build the examples in CI.
* Fixes FormidableLabs#576
* Adds node tests for just the enhancer to get us at least a little bit of SSR testing
* Improves the babel script commands
ryan-roemer and others added 27 commits August 22, 2018 11:55
…false` wherein libraries can indicate their ESM re-exports are side effect free and can be much more efficiently removed for smaller, faster final bundles. (FormidableLabs#965)

Lodash has already rolled out this change in
https://unpkg.com/lodash-es@4.17.5/package.json

## Issues

This was originally uncovered / discussed at length in:

* webpack/webpack#1750

This PR should resolve the issues discussed in:

* FormidableLabs/victory#549
* FormidableLabs/redux-little-router#262

## Changes

* Add `sideEffects: false` to `package.json` to allow webpack4 tree-shaking to actually remove all unused code.

> This PR has been automatically opened by your friendly [`multibot`](https://github.com/FormidableLabs/multibot/). The transform code and documentation is available at: https://github.com/FormidableLabs/multibot-examples/tree/master/transforms/webpack-side-effects
- **Breaking Change**: Radium now exports defaults as `.default`, so for runtimes like Node.js for all files in `lib/**`. We have changed `package.json:main` to point to `/index.js` instead of `/lib/index.js` as a convenience wrapper to expose mostly what was there before so behavior of `const Radium = require('radium');` works mostly as it did before. Caveats:
- Add `es` ESM module export files.
- Fix `package.json:scrtips.postinstall` task to correctly work for git-based dependencies.
- Fixes FormidableLabs#738
- This is hacky, but possibly **acceptably** hacky.
- Detect and transfer instance method properties from arrow functions to a copy of the prototype (where by definition they don't exist) to allow property inheritance call chaining by `RadiumEnhancer` which otherwise doesn't get its methods called.
Bug fix: render an array of elements

* handle array child elements; ensure that extraStateKeyMap is cleaned up

* Add regression test for render children.
* upgrade dev dependencies to latest react, ^16.2.0

* tests for fragments
Had an error wherein the right node version never got installed...
@jaredbrookswhite
Copy link
Contributor Author

updated and done. sorry for the confusion, not in my normal dev environment today.

@kylecesmat kylecesmat merged commit 55a6965 into FormidableLabs:master Aug 22, 2018
@kylecesmat
Copy link
Contributor

This has been released in radium@0.25.0

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet