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

A couple of features and bug fixes #232

Closed
wants to merge 14 commits into from
Closed

Conversation

r0x0r
Copy link

@r0x0r r0x0r commented Sep 15, 2014

Hello there,
I have been testing out Lychee fixing bugs and adding minor features in progress. Here is a list of the changes I have introduced so far.

  • Faster photo browsing thanks to caching.
  • Default sort order of albums is changed from new to old.
  • Preserves scroll position when changing from albums to album and vice versa
  • Better Unicode support for values read from the database

Hopefully these are of use to you. If you have any questions, do not hesitate to ask.

Cheers,
Roman

- The albums view is stored in memory when changing from the albums to the individual album/photo view.
- When viewing photos, the next picture in the current album is preloaded.

* Default sort order of albums is changed from new to old.

* Fixed scroll position when changing from albums to album and vice versa
Conflicts:
	assets/min/main.js
@electerious
Copy link
Owner

That sounds great! Give me some time to look through all those changes.

A few things that need to be fixed/done before:

  • Use tabs for indentation, not spaces
  • Update the code to the current master state

Thanks for your work!

@r0x0r
Copy link
Author

r0x0r commented Sep 16, 2014

Hello,

Sorry about the spaces. Changed them to tabs now.
What do you mean by updating to the current master state, though? I am still learning my ways around Git.

Cheers,
Roman

On 16 Sep 2014, at 13:49, Tobias Reich notifications@github.com wrote:

That sounds great! Give me some time to look through all those changes.

A few things that need to be fixed/done before:

Use tabs for indentation, not spaces
Update the code to the current master state
Thanks for your work!


Reply to this email directly or view it on GitHub.

@electerious
Copy link
Owner

Some parts of Lychee have changed since you forked the project. You need to make sure that your branch is up to date with the master of Lychee before I can merge everything.

modal += "<input id='username' type='text' name='' value='' placeholder='username'>";
modal += "<input id='password' type='password' name='' value='' placeholder='password'>";
modal += "<input id='username' type='text' name='username' value='' placeholder='username'>";
modal += "<input id='password' type='password' name='password' value='' placeholder='password'>";
Copy link
Owner

Choose a reason for hiding this comment

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

Are these changes good for something (Nothing again them, just curious)?

Copy link
Author

Choose a reason for hiding this comment

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

I have been trying to make Chrome to save a password and this was one of solutions, but alas it did not work out. I will investigate this issue.

I have merged with the latest master. Do you want me to create a new pull request?

Cheers,
Roman

On 16 Sep 2014, at 21:34, Tobias Reich notifications@github.com wrote:

In assets/js/build.js:

@@ -195,8 +195,8 @@ build = {
modal += "

Sign In

";
modal += "";
modal += "
";

  •   modal +=            "<input id='username' type='text' name='' value='' placeholder='username'>";
    
  •   modal +=            "<input id='password' type='password' name='' value='' placeholder='password'>";
    
  •   modal +=            "<input id='username' type='text' name='username' value='' placeholder='username'>";
    
  •   modal +=            "<input id='password' type='password' name='password' value='' placeholder='password'>";
    
    Are these changes good for something (Nothing again them, just curious)?


Reply to this email directly or view it on GitHub.

Copy link
Owner

Choose a reason for hiding this comment

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

A new PR including the other changes would be nice.

@electerious
Copy link
Owner

Went through the code. Looks great! Thanks for the work. Some small things here and there and we can merge it :)

@r0x0r
Copy link
Author

r0x0r commented Sep 17, 2014

I have updated the code with the changes proposed by you, plus added another fix for an albums cache issue when renaming an album.

electerious added a commit that referenced this pull request Sep 17, 2014
@electerious
Copy link
Owner

Awesome job! I opened a new v2.6.3 branch with all your changes. Thanks a lot :) Feel free to create new PRs from this branch.

@@ -190,11 +190,12 @@ var lychee = {
album.load(albumID, true);
}
photo.load(photoID, albumID);
photo.preloadNext(photoID,albumID);
Copy link
Owner

Choose a reason for hiding this comment

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

Would be better to preload the next photo after the current photo has been loaded (it should have the higher priority). What do you think?

Copy link
Author

Choose a reason for hiding this comment

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

I agree with you that it is a good idea. The change is now pushed.

On Fri, Sep 19, 2014 at 8:19 PM, Tobias Reich notifications@github.com
wrote:

In assets/js/lychee.js:

@@ -190,11 +190,12 @@ var lychee = {
album.load(albumID, true);
}
photo.load(photoID, albumID);

  •        photo.preloadNext(photoID,albumID);
    

Would be better to preload the next photo after the current photo has been
loaded (it should have the higher priority). What do you think?


Reply to this email directly or view it on GitHub
https://github.com/electerious/Lychee/pull/232/files#r17796876.

@electerious
Copy link
Owner

Found out the the scroll position isn't always correct (Tested with Safari & Chrome). When you are too near to the top, it just goes back to the top and sometimes it has an offset with about -200px to the last position. Any idea?

@r0x0r
Copy link
Author

r0x0r commented Sep 20, 2014

I have noticed this erratic behaviour, but to be honest I have no idea what
causes it. Will see if I can find a solution.

Roman

On Sat, Sep 20, 2014 at 12:47 AM, Tobias Reich notifications@github.com
wrote:

Found out the the scroll position isn't always correct (Tested with Safari
& Chrome). When you are too near to the top, it just goes back to the top
and sometimes it has an offset with about -200px to the last position. Any
idea?


Reply to this email directly or view it on GitHub
#232 (comment).

@electerious electerious mentioned this pull request Oct 9, 2014
electerious added a commit that referenced this pull request Oct 9, 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
2 participants