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

Widget: Fix marquee effect implementation from xiboTextRender #2479

Merged
merged 4 commits into from
Apr 11, 2024

Conversation

rubenberttpingol
Copy link
Contributor

maurofmferrao
maurofmferrao previously approved these changes Apr 10, 2024
@rubenberttpingol
Copy link
Contributor Author

I believe that the deleted lines here are not necessary anymore since we have moved the template animation being handled by onTemplateVisible using xiboLayoutAnimate. But I'm not certain yet if we still need those deleted lines here for module only.

@rubenberttpingol
Copy link
Contributor Author

Can you confirm @maurofmferrao?

@maurofmferrao
Copy link
Member

Can you confirm @maurofmferrao?

I think that makes sense, but we need just to make sure all modules/templates that were using xiboTextRender also run xiboLayoutAnimate.

What happens if we have a template that needs to be animated on onTemplateRender and not on onTemplateVisible? Do we ever want to do that @dasgarner ?

@maurofmferrao
Copy link
Member

We also need to test if the behaviour change on the editor, it might not start the animation on resizing :/

@rubenberttpingol
Copy link
Contributor Author

We also need to test if the behaviour change on the editor, it might not start the animation on resizing :/

Yes, I think this makes sense and I can see it's not animating now on resize. I can add isEditor to its condition

@maurofmferrao
Copy link
Member

We also need to test if the behaviour change on the editor, it might not start the animation on resizing :/

Yes, I think this makes sense and I can see it's not animating now on resize. I can add isEditor to its condition

Not sure we want that, what if in a different widget we want to run on load and not on visible, even on a player?

The question is: why is pauseEffectOnStart false and running those lines since its default value is true?

@rubenberttpingol
Copy link
Contributor Author

We also need to test if the behaviour change on the editor, it might not start the animation on resizing :/

Yes, I think this makes sense and I can see it's not animating now on resize. I can add isEditor to its condition

Not sure we want that, what if in a different widget we want to run on load and not on visible, even on a player?

The question is: why is pauseEffectOnStart false and running those lines since its default value is true?

I think pauseEffectOnStart is being set to false on editor-render.js since it's being set to have a default false value

@rubenberttpingol
Copy link
Contributor Author

No, I think it's also being set to have default value of false on xibo-player.

@maurofmferrao
Copy link
Member

Yeah, it's being set as false on those two places, but maybe the fix is just to set it as true by default?

I think that we want the render to always pause on start except when we force it to start right away, on a specific module or on the editor. So we still need to check if it's the editor to start the animation, maybe do that in editor-render.js instead of in xiboTextRender?

@rubenberttpingol
Copy link
Contributor Author

Yes, I think we should proceed with this.

Copy link
Member

@maurofmferrao maurofmferrao left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@dasgarner
Copy link
Member

TEST: /Layout END

Copy link

Test Suite: succeeded ✅
https://github.com/xibosignage/xibo-cms/actions/runs/8643853682

@dasgarner dasgarner merged commit d2363a8 into develop Apr 11, 2024
2 checks passed
@dasgarner dasgarner deleted the bugfix/westphal-rss-static-widget-marquee-up branch April 11, 2024 09:13
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.

3 participants