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.

Sign up for GitHub

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

Jump to bottom

Fix directorybrower not resetting scroll #17

Merged
merged 3 commits into from
Jan 16, 2019
Merged

Fix directorybrower not resetting scroll #17

merged 3 commits into from
Jan 16, 2019

Conversation

ViXXoR
Copy link
Contributor

@ViXXoR ViXXoR commented Jan 11, 2019 •
edited
Loading

When using the directory browser for adding new libraries, clicking on a folder would not reset the scrollbar to the top of the folder. This fixes that.

@ViXXoR ViXXoR force-pushed the minor-web-ui-enhancements branch from 617e109 to ca0b3f5 Compare January 11, 2019 17:34
@EraYaN
Copy link
Member

EraYaN commented Jan 11, 2019

Merge #19 first, and this one should have no merge conflict anymore.

@ViXXoR ViXXoR changed the title Minor web ui enhancements Fix directorybrower not resetting scroll and imageupload error handling Jan 11, 2019
@joshuaboniface
Copy link
Member

@EraYaN Na, that would make GitHub too smart - still needs a manual fix.

@ViXXoR Are you able to split this out into two PRs? One for the scroll issue, and the other for imageupload. It will make the history and changelog a lot cleaner.

@ViXXoR
Copy link
Contributor Author

ViXXoR commented Jan 12, 2019

Yes, I can. I should have in the first place but I was a little lazy.
I'll do it when I have a chance.

@ViXXoR ViXXoR force-pushed the minor-web-ui-enhancements branch from ca0b3f5 to 8539ba4 Compare January 13, 2019 00:38
@ViXXoR ViXXoR changed the title Fix directorybrower not resetting scroll and imageupload error handling Fix directorybrower not resetting scroll Jan 13, 2019
@ViXXoR
Copy link
Contributor Author

ViXXoR commented Jan 13, 2019

Ok I've split it out, the imageuploader changes are now in #29

@LeoVerto
Copy link
Contributor

This PR contains a lot of cleanup including quotation mark changes. I personally feel like all of that should be a seperate one.

Have we even agreed on code style yet?

@ViXXoR
Copy link
Contributor Author

ViXXoR commented Jan 13, 2019

I went and matched the style from the massive changes in PR #1, so I think it should be safe, at least for now?

dkanada
dkanada approved these changes Jan 13, 2019
View reviewed changes
Copy link
Member

@dkanada dkanada left a comment

Choose a reason for hiding this comment

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

Quick note, there seems to be around six commits that were pulled in that are still causing some merge issues. They have already been commited to master in other pull requests so you could just drop them on your branch. Otherwise the changes look good to me!

@ViXXoR ViXXoR force-pushed the minor-web-ui-enhancements branch from 8539ba4 to 806cc3e Compare January 13, 2019 16:04
@ViXXoR
Ensure Directory Browser Scrolls to Top on Selection
10d5c28 
When a user chooses a directory using the directory browser, it previously stayed at its scroll location. Now it moves back to the top, which is expected behaviour.
@ViXXoR ViXXoR force-pushed the minor-web-ui-enhancements branch from 806cc3e to b9a1709 Compare January 13, 2019 16:06
@ViXXoR
Copy link
Contributor Author

ViXXoR commented Jan 13, 2019

I've cleaned it up and rebased, should be a good clean merge now.

nvllsvm
nvllsvm previously requested changes Jan 13, 2019
View reviewed changes
Copy link
Member

@nvllsvm nvllsvm left a comment

Choose a reason for hiding this comment

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

Keep double quotes - it's consistent with many of the other files in this repo.

@ViXXoR
Copy link
Contributor Author

ViXXoR commented Jan 13, 2019

Keep double quotes - it's consistent with many of the other files in this repo.

All the changes from #1 change them to single quotes. This is just going to be converted to match them at some point...

@nvllsvm
Copy link
Member

nvllsvm commented Jan 13, 2019

I obviously didn't read :|

@ViXXoR
Copy link
Contributor Author

ViXXoR commented Jan 14, 2019

So am I going to need to make changes to get this accepted? Or are the quotes fine matching #1?

JustAMan
JustAMan approved these changes Jan 14, 2019
View reviewed changes
Copy link
Contributor

@JustAMan JustAMan left a comment

Choose a reason for hiding this comment

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

Mostly space formatting nitpicking. If you accept these please squash them!

Plus two more de-uglifications suggested.

Other than that looks good.

@ViXXoR ViXXoR force-pushed the minor-web-ui-enhancements branch from b9a1709 to 4819fc9 Compare January 14, 2019 18:02
@ViXXoR
Copy link
Contributor Author

ViXXoR commented Jan 14, 2019

Mostly space formatting nitpicking. If you accept these please squash them!

Plus two more de-uglifications suggested.

Other than that looks good.

I made all your changes and just pushed myself to save the commits. Thanks for the going through it.

@joshuaboniface joshuaboniface dismissed nvllsvm’s stale review January 16, 2019 17:04

Issue addressed in discussion, standards were changed in #1

@joshuaboniface joshuaboniface merged commit 58ea56f into jellyfin:dev Jan 16, 2019
@ViXXoR ViXXoR deleted the minor-web-ui-enhancements branch January 19, 2019 20:06
@joshuaboniface joshuaboniface mentioned this pull request Jan 21, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JustAMan JustAMan JustAMan approved these changes

@joshuaboniface joshuaboniface joshuaboniface approved these changes

@dkanada dkanada dkanada approved these changes

@nvllsvm nvllsvm nvllsvm left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@ViXXoR @EraYaN @joshuaboniface @LeoVerto @nvllsvm @JustAMan @dkanada