BREAKING(Transition): renamed Transition to Animation and added docs for the animations #505

mnajdova · 2018-11-20T20:34:35Z

This PR is adding docs about how the animations should be defined in the theme, and used together with the Transition component, as well as the animation property.

Update

As part of this PR, I am going to also rename the Transition component to Animation, and the animationName property to name, following the review comments.

codecov · 2018-11-20T20:41:11Z

Codecov Report

Merging #505 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master     #505   +/-   ##
=======================================
  Coverage   88.17%   88.17%           
=======================================
  Files          42       42           
  Lines        1455     1455           
  Branches      187      187           
=======================================
  Hits         1283     1283           
  Misses        167      167           
  Partials        5        5

Impacted Files	Coverage Δ
src/components/Animation/Animation.tsx	`90.47% <100%> (ø)`
src/index.ts	`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 eb583a7...dbcfcd7. Read the comment docs.

miroslavstastny

The first question which comes to my mind is *when I should use Transition and when animation? In the original animation PR, there was a clear description and comparison of the two. Would it make sense to add similar description here as well?

miroslavstastny · 2018-11-20T20:38:43Z

docs/src/views/Theming.tsx

+      <NavLink to="components/provider">Provider</NavLink>.
+    </p>
+    <p>
+      This is done with the Provider's <code>theme</code> prop. The animations are then are applied


The animations are then are applied
remove the second 'are'

miroslavstastny · 2018-11-20T20:41:03Z

docs/src/views/Theming.tsx

+    <p>
+      This is done with the Provider's <code>theme</code> prop. The animations are then are applied
+      based on their name by using the <NavLink to="components/transition">Transition</NavLink>{' '}
+      component, or the <code>animation</code> property available on all UI component. Here's how we


nit: all UI component - I would rather write something like all Stardust components

levithomason · 2018-11-21T08:52:16Z

Note, PR title would be "docs(theming):..."

levithomason · 2018-11-21T08:57:18Z

docs/src/views/Theming.tsx

+      ].join('\n')}
+      render={() => (
+        <div>
+          <Transition animationName="spinner">


This strikes me as odd, "Transition animationName=..." Should we rename this "Transition name=..." or "Animation name=..."?

Thanks for the suggestion, I will rename the Transition component to Animation, as that is the word we use for describing this behavior everywhere else. Transition was just picked up because of SUIR. Then we can safely rename the animationName prop to name. I will add this changes to this PR too.

levithomason · 2018-11-21T08:58:31Z

docs/src/views/Theming.tsx

@@ -208,6 +208,82 @@ export default () => (
      <NavLink to="components/provider">Provider</NavLink> at the root of your app.
    </p>

+    <Header as="h2" content="Animations" />
+    <p>
+      Another important part of the theming in Stardust are the <code>animations</code>. You can


Another important part of the theming in Stardust are the animations.

This line doesn't add any value to the user's goal. Suggest removing it.

levithomason · 2018-11-21T09:01:10Z

docs/src/views/Theming.tsx

+    <Header as="h2" content="Animations" />
+    <p>
+      Another important part of the theming in Stardust are the <code>animations</code>. You can
+      define the animations in a very similar way as you would define them using css, by providing a


Suggest:

You define animations in Stardust in a very similar way to CSS, by providing keyframes and animation properties.

Then show a minal provider code snippet immediately following this.

-addressed comments for the docs

miroslavstastny · 2018-11-22T16:36:34Z

docs/src/examples/components/Animation/Types/AnimationExample.tsx

+import { Animation, Icon } from '@stardust-ui/react'
+
+const AnimationExample = () => (
+  <Animation name="spinner">


All the Animation examples are theme dependent. Would it make more sense to add Provider to the examples and explicitly define the animations there?
Not sure about that, would like to hear other opinions.

I understand your point, but I don't think this is good idea, because we will have much code in the examples, for something that is documented in the Theming guide. How the animation are created will be document as part of the theming guidem and the default theme will have few animations that will be shown in the examples. Let's hear the opinion from the other folks too. @kuzhelov @Bugaa92 @alinais @levithomason @layershifter

I have a similar problem in #451, we definitely need to show theme specific stuff because examples can be broken when you will try to switch to another theme.

I understand your point, but I don't think this is good idea, because we will have much code in the examples, for something that is documented in the Theming guide.

This is a valid point, too.

After offline discussion we decided to add the Provider with the theme. All examples as well as the Theming.tsx guide page, now contains a Provider where the animations are defined.

miroslavstastny · 2018-11-22T16:38:18Z

docs/src/views/Theming.tsx

+    <ExampleSnippet
+      value={[
+        `<Animation name="spinner" delay="2s" duration="1s"><Icon name="user" circular /></Animation>`,
+        `<Icon name="book" animation="spinner" delay="5s" duration="2s" circular/>`,


animation here should be an object:

<Icon name="book" animation={{name: "spinner", delay:"5s", duration:"2s"}} circular/>

Right, thanks for the catch!

docs/src/views/Theming.tsx

miroslavstastny · 2018-11-22T16:39:55Z

docs/src/views/Theming.tsx

+          <Animation name="spinner" delay="2s" duration="1s">
+            <Icon name="user" circular />
+          </Animation>
+          <Icon name="book" animation="spinner" delay="5s" duration="2s" circular />


animation -> object

miroslavstastny · 2018-11-22T16:40:55Z

docs/src/views/Theming.tsx

+
+    <p>
+      The difference between using the Animation component versus the animation property is that,
+      the Animation component can be safely use for applying animations on{' '}


use -> used

-renamed Animation theme interface to AnimationProp, because of conflicts with the Animation component name

layershifter · 2018-11-26T08:51:40Z

docs/src/examples/components/Animation/Types/AnimationExampleDelay.tsx

+
+const AnimationExampleDelay = () => (
+  <div>
+    {'This animation will start after 5 seconds'}


Suggested change

{'This animation will start after 5 seconds'}

This animation will start after 5 seconds

Are brackets necessary?

…e and removed it form the teams' theme

# Conflicts: # CHANGELOG.md

-added documentation for the way the animations are defined in the theme

24a5d5d

mnajdova requested review from dvdzkwsk, kuzhelov, levithomason and miroslavstastny as code owners November 20, 2018 20:34

mnajdova added 📚 docs 🚀 ready for review labels Nov 20, 2018

miroslavstastny reviewed Nov 20, 2018

View reviewed changes

levithomason reviewed Nov 21, 2018

View reviewed changes

mnajdova changed the title ~~feat(docs): Adding docs for defining the animations in the theme~~ feat(theming): Adding docs for defining the animations in the theme Nov 21, 2018

manajdov added 4 commits November 22, 2018 12:32

-renamed Transition to Animation, renamed animationName to name

d42daa2

-addressed comments for the docs

-added Animation to the theme typings

5435582

Merge branch 'master' into doc/animation-theme

928848f

-updated changelog

96828ad

mnajdova changed the title ~~feat(theming): Adding docs for defining the animations in the theme~~ BREAKING(animation): renamed Transition to Animation and Added docs for defining the animations in the theme Nov 22, 2018

mnajdova changed the title ~~BREAKING(animation): renamed Transition to Animation and Added docs for defining the animations in the theme~~ BREAKING(animation): renamed Transition to Animation and added docs for defining the animations in the theme Nov 22, 2018

miroslavstastny reviewed Nov 22, 2018

View reviewed changes

-addressed PR comments

86668e5

-renamed Animation theme interface to AnimationProp, because of conflicts with the Animation component name

layershifter reviewed Nov 26, 2018

View reviewed changes

manajdov added 3 commits November 26, 2018 11:21

-removed unnecessary brackets

5ec08a5

-added Provider with animations in the examples and theming guide pag…

f06bfe8

…e and removed it form the teams' theme

Merge branch 'master' into doc/animation-theme

2d6cf5a

mnajdova changed the title ~~BREAKING(animation): renamed Transition to Animation and added docs for defining the animations in the theme~~ BREAKING(animation): renamed Transition to Animation and added docs Nov 27, 2018

mnajdova changed the title ~~BREAKING(animation): renamed Transition to Animation and added docs~~ BREAKING: renamed Transition to Animation and added docs for the animations Nov 27, 2018

mnajdova changed the title ~~BREAKING: renamed Transition to Animation and added docs for the animations~~ reafactor: renamed Transition to Animation and added docs for the animations Nov 27, 2018

mnajdova changed the title ~~reafactor: renamed Transition to Animation and added docs for the animations~~ breaking(animations): renamed Transition to Animation and added docs for the animations Nov 27, 2018

mnajdova changed the title ~~breaking(animations): renamed Transition to Animation and added docs for the animations~~ breaking(Transition): renamed Transition to Animation and added docs for the animations Nov 27, 2018

Merge branch 'master' into doc/animation-theme

dbcfcd7

# Conflicts: # CHANGELOG.md

miroslavstastny approved these changes Nov 27, 2018

View reviewed changes

mnajdova changed the title ~~breaking(Transition): renamed Transition to Animation and added docs for the animations~~ feat(Transition): renamed Transition to Animation and added docs for the animations Nov 27, 2018

mnajdova changed the title ~~feat(Transition): renamed Transition to Animation and added docs for the animations~~ BREAKING(Transition): renamed Transition to Animation and added docs for the animations Nov 27, 2018

mnajdova merged commit 49362ad into master Nov 27, 2018

layershifter deleted the doc/animation-theme branch January 10, 2019 11:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BREAKING(Transition): renamed Transition to Animation and added docs for the animations #505

BREAKING(Transition): renamed Transition to Animation and added docs for the animations #505

mnajdova commented Nov 20, 2018 •

edited

Loading

codecov bot commented Nov 20, 2018 •

edited

Loading

miroslavstastny left a comment

miroslavstastny Nov 20, 2018

miroslavstastny Nov 20, 2018

levithomason commented Nov 21, 2018

levithomason Nov 21, 2018

mnajdova Nov 22, 2018 •

edited

Loading

levithomason Nov 21, 2018

levithomason Nov 21, 2018

miroslavstastny Nov 22, 2018

mnajdova Nov 23, 2018

layershifter Nov 26, 2018

mnajdova Nov 26, 2018

miroslavstastny Nov 22, 2018

mnajdova Nov 23, 2018

miroslavstastny Nov 22, 2018

miroslavstastny Nov 22, 2018

layershifter Nov 26, 2018

mnajdova Nov 26, 2018

	{'This animation will start after 5 seconds'}
	This animation will start after 5 seconds

BREAKING(Transition): renamed Transition to Animation and added docs for the animations #505

BREAKING(Transition): renamed Transition to Animation and added docs for the animations #505

Conversation

mnajdova commented Nov 20, 2018 • edited Loading

Update

codecov bot commented Nov 20, 2018 • edited Loading

Codecov Report

miroslavstastny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

levithomason commented Nov 21, 2018

Choose a reason for hiding this comment

mnajdova Nov 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mnajdova commented Nov 20, 2018 •

edited

Loading

codecov bot commented Nov 20, 2018 •

edited

Loading

mnajdova Nov 22, 2018 •

edited

Loading