Skip to content

Conversation

tyom
Copy link
Contributor

@tyom tyom commented Oct 6, 2013

This is something I've been doing in my own projects for a while now by using a local modified version. I thought I would be good to share it with others.

This PR adds an option to animate reload transitions. When LiveReload detects changes and reloads CSS a short transition of style changes is applied to all elements right after the reload happens. I find it easier to detect changes and it smoothes the workflow (less jarring).

I have created a pull request in the original LiveReload repo a while ago but haven't heard from the author. Judging by the age of other pull requests and the last commit date to that project I don't have high hopes for it to ever make it. And since this project hosts its own version or LR, and is the dependency to other package I use (grunt-contrib-watch), it would be great to get this merged here first. Later I can create separate PRs for grunt-contrib-watch and connect-livereload to add options to enable the animation from wherever they are called.

This is animation is off by default so no one will see any difference unless they explicitly enable it.

@blcarson
Copy link

blcarson commented May 5, 2014

Any progress on this?

@tyom
Copy link
Contributor Author

tyom commented May 5, 2014

Hi. Sorry for the late response.

The animate option is disabled by default. I can appreciate that people who currently use it may not like it, but those who do can enable it. You can enable it there to see how it works.

Unfortunately, connect-livereload that many people may use (e.g. Yeoman's default webapp generator) only allows configuration of the port and doesn't seem to pass any other livereload options down to LiveReload script. So it would need a separate pull request for that.

The dependency chain goes like this: tiny-lr > grunt-contrib-watch > connect-livereload. So I think it needs to be done from the bottom up.

@mklabs
Copy link
Owner

mklabs commented May 10, 2014

Hi @tyom

Currently seeing if we can merge your pull request, but the delay and the other merges done to upstream make it not mergable without a rebase.

I've created a livereload-animate branch if you'd like to target your PR on it.

@tyom
Copy link
Contributor Author

tyom commented May 11, 2014

Hi @mklabs,

It won't be easy to rebase against my changes since I worked with original LiveReload CoffeeScript file which I then compiled and added the animation features (see my commits). Since my PR was created there was a number of changes to the compiled JS file in your repo which is now a separate fork of the original LiverReload script.

I could reimplement the feature in your compiled version of course, but what concerns me is that we're now working on the snapshot of LiveReload as of a year ago and any changes made to the original will be incompatible with this version. For example, it has been regenerated 8 days ago to 'Improve logging'.

I think it would be a good idea to not make changes directly to the compiled version of livereload.js and always work with the original CoffeeScript files, rebuilding livereload.js with every change.

@mklabs
Copy link
Owner

mklabs commented May 11, 2014

Hey @tyom

Thank you for the detailed response. Sorry, I really don't want to burden you, I know it can be really tedious to manage this kind of upstream / rebase stuff.

You did the right thing, by editing the original coffee files and compiling back. We only host the compiled version, have diverged from the original and official repository for livereload.js etc.

It brings a lot of complexity. I really think that hosting the livereload.js script is bringing more harm than good.

I'm considering to switch back to another system, something that lets user choose the livereload.js script to serve at /livereload.js path.

I think it's alreay doable, it just needs to be documented (we can define and should be able to override the GET /livereload.js route)


Back to the original PR context.. That being said, I really want to merge your changes if you need them here. We can work out a new PR, with just the changes to the compiled version.

We'll sort out the situation by improving the build process (sometime soon) if we need to maintain changes to the lr.js script.

Let me know if I can help in any way.

@tyom
Copy link
Contributor Author

tyom commented May 14, 2014

Hi @mklabs,

I created a new branch with extracted changes to add transitions. You can test it out by changing this.animate = false to true.

Let me know if you want me to create a new pull request.

@mklabs
Copy link
Owner

mklabs commented May 15, 2014

Hi @tyom

This is great! Thank you.

I'll review and merge in the next few days. The PR is welcome, but I can do without. I don't think you can edit the source branch after the PR have been created.

@tyom
Copy link
Contributor Author

tyom commented May 15, 2014

I've created a new PR. Closing this in favour of #43.

@tyom tyom closed this May 15, 2014
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.

4 participants