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

Dynamic Background Animation #2092

Merged
merged 6 commits into from
Mar 23, 2018
Merged

Conversation

anojht
Copy link
Contributor

@anojht anojht commented Mar 23, 2018

Implemented a background fade in/fade out on a ten second interval to cycle backgrounds on landing and login pages.

@tidusjar
Copy link
Member

The build is failing on some of the linting tasks.

ClientApp/app/settings/settings.module.ts[143, 32]: file should end with a newline

@anojht
Copy link
Contributor Author

anojht commented Mar 23, 2018

@tidusjar Yes I think this is because the file in the develop branch does not end with a newline. Just double checked the branch from my fork and it's not causing the linting error.

@tidusjar
Copy link
Member

My bad! i'll take a look

}

public cycleBackground() {
setTimeout(() => {
Copy link
Member

Choose a reason for hiding this comment

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

So the this.timer calls this.cycleBackground every 10 seconds, and then we have another two timers in here?

Once that clears the background and another that sets it. Why do we have this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previously I was running into sync issues therefore I added the timeout timers. With the new improvements I made they are redundant, will quickly push a new commit with the timers removed.

Copy link
Member

@tidusjar tidusjar left a comment

Choose a reason for hiding this comment

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

See comments

Copy link
Contributor Author

@anojht anojht left a comment

Choose a reason for hiding this comment

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

Addressed the review request and made necessary changes

@tidusjar tidusjar merged commit af319cf into Ombi-app:develop Mar 23, 2018
@tidusjar
Copy link
Member

Nice!

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.

2 participants