Skip to content

Conversation

@proud-nerd
Copy link
Contributor

Resolves #570

[ngClass]="playBackState == PlayBackStates.FastForward ? 'button-active':''"
(click)="fastForward()"
title="fast auto play"></span>
<span class="oi btn-group highlight control-button"
Copy link
Owner

Choose a reason for hiding this comment

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

oi only needed for the icon tag

data-bs-auto-close="outside"
aria-controls="dropdown-basic">
<span class="text-white pe-1">{{selectedPlayBackDuration + 's'}}</span>
<span class="oi-clock"></span>
Copy link
Owner

Choose a reason for hiding this comment

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

so the oi is needed here

Copy link
Owner

Choose a reason for hiding this comment

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

never mind. Ill fix it.

@bpatrik bpatrik merged commit 50b55c2 into bpatrik:master Dec 17, 2022
@bpatrik
Copy link
Owner

bpatrik commented Dec 17, 2022

looks good to me. thank you for your contribution

bpatrik added a commit that referenced this pull request Dec 17, 2022
Also fixing playback issues
@bpatrik
Copy link
Owner

bpatrik commented Dec 17, 2022

I found the design of the speed selector to be inconsistent with design of the app, so I moved to the submenu on the top.

Sorry about that. Please consult next time before sending a PR. Thanks again for the contribution.

@proud-nerd
Copy link
Contributor Author

Thanks for merging and fixing those things you highlighted! I had some trouble aligning the dropdown with those other buttons and 'io' added some weird position offset that affected height. Just got it to work like that, but I like the position you moved it to a lot, so doesn't really matter anymore I guess.

I'm happy to contribute, this was actually my first time ever 🥳 What do you mean by 'consult first'? I thought opening a PR and an issue is just that.

I do have a few improvements that I'd like to see. You removed some logic by refactoring the dropdown at a new location. Should I open separate issues for that or would you like to discuss those here and I open issues/PRs afterwards?

What I'd like to re-add:

  1. Set cookie for playback speed. Especially since it's now in the submenu this would be very handy, because it's a little hidden now and would be great as a set-and-forget kind of setting. At least per client and if possible I'd set expires to 10 years or something, but I could not get that to work, at maximum the cookie was created with 7 days validity.

This feature was there already, but since you are using two-way-data binding now, my function that also set a cookie isn't called anymore. I have no idea why changing playback speed works while having an active slideshow running. The logic you added is also in that function and I just can't see it called anywhere and if this works, the cookie should be set aswell. Maybe you see something I don't.

  1. Selecting a playback speed should start the slideshow (arguable, but I personally think if someone is setting a playback speed, they most likely want to start it next)

Thank you for your work on this! I'm more than happy to fix those things myself and create a PR after we decided what to do

@bpatrik
Copy link
Owner

bpatrik commented Dec 18, 2022

Hi

By consulting I meant it is better to have some back and forth mail before someone puts effort into implementing something. (I hope I did not sound harsh, that was no my intention).
The main reason behind it is that it's basically me only who has the whole code base in their head.
Also I'm relatively picky on the design. Like I won't submit half solutions:)

About the issues:

  1. Sorry I did not see that the change broke the cokie settings. I will need to double check if directory ordering uses local storage. If yes this feature should also do so.

  2. I'm not 100% that settings is at the right place, but could figure out any better . I might move it back to the play button but just something super minimalistic. Or make the play button behave like the FB like button where longpress/hover opens the other like options.
    I do not see this feature to be used often. As you said it should be set and forget. I think the default value should be also set in the config

bpatrik added a commit that referenced this pull request Dec 18, 2022
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.

[Feature] Changing slideshow playback speed

2 participants