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

AnimateCSS #3113

Merged
merged 26 commits into from
Sep 8, 2023
Merged

AnimateCSS #3113

merged 26 commits into from
Sep 8, 2023

Conversation

bugsounet
Copy link
Contributor

@bugsounet bugsounet commented May 28, 2023

Hi,

This is my testing code for AnimateCSS for show(), hide(), updateDom()

Naturally, we have to do better !

I voluntarily modify newsfeed and compliments in order to test

Note: I will correct checks later... it's a test...

@bugsounet
Copy link
Contributor Author

ok... I will solve localy :/

@codecov-commenter
Copy link

codecov-commenter commented May 28, 2023

Codecov Report

Merging #3113 (2d5effc) into develop (9d49196) will decrease coverage by 0.49%.
The diff coverage is 1.03%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@             Coverage Diff             @@
##           develop    #3113      +/-   ##
===========================================
- Coverage    26.15%   25.66%   -0.49%     
===========================================
  Files           53       54       +1     
  Lines        11502    11760     +258     
===========================================
+ Hits          3008     3018      +10     
- Misses        8494     8742     +248     
Files Changed Coverage Δ
js/animateCSS.js 0.00% <0.00%> (ø)
js/loader.js 0.00% <0.00%> (ø)
js/main.js 0.00% <0.00%> (ø)
js/module.js 68.24% <75.00%> (ø)

... and 2 files with indirect coverage changes

@bugsounet
Copy link
Contributor Author

bugsounet commented May 28, 2023

~ Issue #3111 ~

So...

I purpose to you to make a test with newsfeed module
just change lines 304 and 308 with : this.updateDom(this.config.animationSpeed, 3, 14);

syntax: updateDom(speed, animateOut, animateIn)
animateOut, animateIn number value are available there and it's the result of the animated names from the Animate.style website

Result: The news will come from the left, wait in the middle and come out the right

Note: I repeat, it's a testing code, and need to be really merged properly with hideModule, showModule function of main.js

@khassel
Copy link
Collaborator

khassel commented Jun 5, 2023

Result: The news will come from the left, wait in the middle and come out the right

tested this with the same result

@bugsounet
Copy link
Contributor Author

@rejas says Start with a very simple animation so we can discuss the actual implementation?

I know, I have to do better implementation
It's just a testing code actually.

Should I do better? (Of course)
Are you really interested in this optional feature?

@rejas
Copy link
Collaborator

rejas commented Jun 6, 2023

@rejas says Start with a very simple animation so we can discuss the actual implementation?

I know, I have to do better implementation It's just a testing code actually.

Should I do better? (Of course) Are you really interested in this optional feature?

Sorry for not being more active on your PR, I really like the feature but with a newborn I cant concentrate right now much on these PRs (contrary to small stuff like dependency updates that dont take much brain power)

Expect more feedback next week, then I might have more time to think about it.

@bugsounet
Copy link
Contributor Author

@rejas: congratulations to you ! This is happiness !

Take your time, a newborn is more important than this PR (I know the time and energy it takes!)

@bugsounet
Copy link
Contributor Author

I will try to do something cleaner because a test code is not made to be in production

@bugsounet
Copy link
Contributor Author

bugsounet commented Jun 10, 2023

I realize that it's not that easy to do something cleaner

@rejas, Veeck when you have some time, can you inspect ?
It's always better to have another opinion

@rejas
Copy link
Collaborator

rejas commented Jun 10, 2023

just change lines 304 and 308 with : this.updateDom(this.config.animationSpeed, 3, 14);

Would it be possible to have something more "verbose" when calling the updateDom? Something more like this.updateDom(this.config.animationSpeed, Animate.Bounce, Animate.FadeOut);
which would make it more readable for developers?

Also what do you think about having the module animation configurable (just like position):

		{
			module: "clock",
			position: "top_left",
                        animateIn: "bounce",
                        animateOut: "fade_out",
		},

?

As for the cleanliness of the code: I dont mind testing code in there, as long as the API to the modules is well-thought of. The actual implementation can be changed easily later, the API stays for a long time :-)

@bugsounet
Copy link
Contributor Author

Oh yes!!! very good idea.
I will try to do something about it

You prefer Animate.Bounce , Animate.FadeOut, Animate.xxx
I will create an object to manage this then

On the other hand, what bothers me, if we put all the animations in the same object that would mean that we can use the "animateOut" in the "animateIn" and vice versa
That's why I separated the animations

I think you would like that if the name of the requested animation does not exist, we fall back on the default MM² animation

Warning:

  • don't forget that animations add a class at the module position (it's add animate__animated <animation name> classes)
  • this necessarily breaks the test suite
  • You will have to review your test suite :/

…nfig

* add animateIn and animateOut in module definition
* use directly the name of the animation
* option.animate use the name of animation
* can't be override with option.animate if animateIn / animateOut are
defined in module definition
@bugsounet
Copy link
Contributor Author

(sometime e2e scares me...)

@bugsounet
Copy link
Contributor Author

bugsounet commented Jun 11, 2023

Just because humm... I'm crazy !

I just add possibility to use animateIn on start of MM²

@rejas : what do you think about that ?

Result:

@bugsounet
Copy link
Contributor Author

bugsounet commented Sep 2, 2023

@rejas : Veeck,
this must be good for me

If you want to see again ;)

can I start writing the doc?

js/animateCSS.js Outdated Show resolved Hide resolved
js/animateCSS.js Outdated Show resolved Hide resolved
js/main.js Outdated Show resolved Hide resolved
js/main.js Outdated Show resolved Hide resolved
js/module.js Outdated Show resolved Hide resolved
@rejas
Copy link
Collaborator

rejas commented Sep 5, 2023

sorry for the long turnaroundtime, lots of stuff to do and the cold is already creeping in...

* (animateCSS.js) `_AnimateCSSIn` -> `AnimateCSSIn`
 * (animateCSS.js) `_AnimateCSSOut` -> `AnimateCSSOut`
* (main.js) Rename `AnimateCSSOut` and `AnimateCSSIn` in accord with
animateCSS.js
 * (main.js) Add Log.debug in animate functionality
 * (main.js) correct var haveAnimateName to let haveAnimateName
* (main.js) Add options object for updateDom() for control speed and
animate
* (module.js) change value name speed -> updateOptions for more
visibility and in accord with new functionality
@bugsounet
Copy link
Contributor Author

no problem, I'm just doing my best for the next release :)

Note: when #3179 is fixed, can you retry test suite?
It's works in local but...

Thx Veeck

@bugsounet bugsounet requested a review from rejas September 6, 2023 18:51
@KristjanESPERANTO
Copy link
Contributor

With #3181 the test shouldn't fail anymore.

I'm looking forward to this feature. Thanks @bugsounet 🙂

@rejas
Copy link
Collaborator

rejas commented Sep 7, 2023

Merged develop myseld, lets see the test results :-)

@bugsounet
Copy link
Contributor Author

Happy crazy test! 😁

js/main.js Show resolved Hide resolved
@rejas rejas added this to the 2.24 milestone Sep 7, 2023
@rejas
Copy link
Collaborator

rejas commented Sep 8, 2023

Ah, screw it, will merge it, we can cleanup further later if necessary (and I dont want to have @bugsounet waiting longer). This is too nice to not have, thx a lot for your work!

@rejas rejas merged commit a92b3d3 into MagicMirrorOrg:develop Sep 8, 2023
@bugsounet bugsounet deleted the develop branch September 8, 2023 07:46
@bugsounet
Copy link
Contributor Author

I will start to write MagicMirror Doc

Note: It’s a shame there isn’t a develop branch on the doc
To see the result of the formatting once written (and correct if necessary)

@rejas
Copy link
Collaborator

rejas commented Sep 8, 2023

That's a good idea to have a develop branch, where we could collect the changes until the next release. Then we would merge it into the main branch.

@bugsounet
Copy link
Contributor Author

we will also have to see with @MichMich
it would be perfect if we had a "point of view" develop of this branch

something like "https://dev-docs.magicmirror.builders/" to see develop branch result
just for see if formating is correct before merging it

@MichMich
Copy link
Collaborator

MichMich commented Sep 8, 2023

I've added a develop branch to the docs repo which is synced with https://develop.docs.magicmirror.builders. Let me know if there is anything else I can do.

@bugsounet
Copy link
Contributor Author

Thx @MichMich,
It's perfect for the moment !

@bugsounet
Copy link
Contributor Author

@rejas:

I see this bugs:

  • AnimateCSS merge hide() and show() animated css class when we do multiple call

--> result it will stay en hide state

I think event listener (is animateCSS file) is not a proper solution
I will correct soon as possible

rejas pushed a commit that referenced this pull request Sep 19, 2023
… multiple call (#3200)

PR: #3113  

I see this bugs:

AnimateCSS merge hide() and show() animated css class when we do
multiple call
--> result it will stay en hide state

I think event listener (is animateCSS file) is not a proper solution

I correct it with like traditional code with timer

Fix too: AnimateIn on first start
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.

6 participants