search

wp-pwa / saturn-app-theme-worona

Saturn theme for worona apps
1 stars 0 forks source link

Change urls on Slider left and right #90

Closed luisherranz closed 7 years ago

luisherranz commented 7 years ago

@orballo I saw you started working on this as well. I also had a branch but you can continue with it.

I thinks the best way to approach this would be in the connection package. I'd define an action and type called ROUTE_CHANGE_REQUESTED which gets type and id as arguments. Then, I'd do a saga listening for those actions which uses Router.push from @worona/next/router to change the url and has the same logic than the Link component of connection, retrieving the necessary permalink for the as parameter from the state. Finally, in the saturn theme, I'd use a saga to listen to all the ACTIVE_POST_SLIDE_CHANGED and dispatch the ROUTE_CHANGE_REQUESTED accordingly. For that, I'd include the postId in the activePostSlideChanged action.

Don't take into account my branch because I was about to implement this in a more complex way.

orballo commented 7 years ago

Actually, I wasn't working on this. My branch (change-slide) was created because, right now, when you open a post from PostList, it doesn't open that Post, but the one with the activeSlide index that it's already in the store. This is because the Link component won't accept an onClick callback, and there is where we were sending the action ACTIVE_POST_SLIDE_CHANGE_REQUESTED.

That said, I can work on this issue as well. Just let me know if I should start with it.

luisherranz commented 7 years ago

Ok! Well, you can work on this then 😁

About that: Instead of sending an action, why don't you initialize the Slide on the correct post ID using the new getId selector from the router?

orballo commented 7 years ago

Yep, I was thinking about how to make it render properly the first time, without changing the index. I'll try that.

luisherranz commented 7 years ago

Great! Let me know how it goes.

On Mon, Aug 21, 2017, 08:42 Eduardo Campaña notifications@github.com wrote:

Yep, I was thinking about how to make it render properly the first time, without changing the index. I'll try that.

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/worona/saturn-app-theme-worona/issues/90#issuecomment-323659269, or mute the thread https://github.com/notifications/unsubscribe-auth/ADJvunWbJRDjgAfS8XxNvsF-awM3S7YSks5saSbCgaJpZM4O7pld .

orballo commented 7 years ago

I'm going to start working on this issue, but I don't understand why do you want me to create an ROUTE_CHANGE_REQUESTED on connection if there is already one in router. Can I not use that one?

luisherranz commented 7 years ago

There's not, there's only a STARTED and FINISHED actions. But you are right, it could live in the router instead. Actually, router knows about type and id so... Maybe Link should also be in the router. Feel free to move all that to the router.

On Mon, Aug 21, 2017, 11:44 Eduardo Campaña notifications@github.com wrote:

I'm going to start working on this issue, but I don't understand why do you want me to create an ROUTE_CHANGE_REQUESTED on connection if there is already one in router. Can I not use that one?

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/worona/saturn-app-theme-worona/issues/90#issuecomment-323698265, or mute the thread https://github.com/notifications/unsubscribe-auth/ADJvuoq3EXsEPGvkWybB8vMRHma-G82Dks5saVGXgaJpZM4O7pld .

orballo commented 7 years ago

Maybe I'm looking somewhere else but:

https://github.com/worona/worona-pwa/blob/115ee50e458d7d748cb510d1798a90cfd2c24b5c/core/router/actions/index.js#L1-L17

https://github.com/worona/worona-pwa/blob/115ee50e458d7d748cb510d1798a90cfd2c24b5c/core/router/types/index.js#L1-L3

So far, with those actions, I've got the route change working, but it breaks when slides are passed too fast.

luisherranz commented 7 years ago

Ok. It looks like I created the action, but with asPath instead of type and id (like Link). Don't worry too much if you don't get it working perfectly. We'll look at it next week.

On Mon, Aug 21, 2017, 15:12 Eduardo Campaña notifications@github.com wrote:

Maybe I'm looking somewhere else but:

https://github.com/worona/worona-pwa/blob/115ee50e458d7d748cb510d1798a90cfd2c24b5c/core/router/actions/index.js#L1-L17

https://github.com/worona/worona-pwa/blob/115ee50e458d7d748cb510d1798a90cfd2c24b5c/core/router/types/index.js#L1-L3

So far, with those actions, I've got the route change working, but it breaks when slides are passed too fast.

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/worona/saturn-app-theme-worona/issues/90#issuecomment-323741071, or mute the thread https://github.com/notifications/unsubscribe-auth/ADJvummuk4yWriATdH9UagYyy8NqHp9Lks5saYJBgaJpZM4O7pld .

luisherranz commented 7 years ago

@orballo what's still not working here?

orballo commented 7 years ago

@luisherranz It's working, but the actions are sent when the animation starts, so the animation is not fluid.

luisherranz commented 7 years ago

Ok, thanks. We'll leave it for now then.

On Tue, 29 Aug 2017 at 17:54 Eduardo Campaña notifications@github.com wrote:

@luisherranz https://github.com/luisherranz It's working, but the actions are sent when the animation starts, so the animation is not fluid.

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/worona/saturn-app-theme-worona/issues/90#issuecomment-325709247, or mute the thread https://github.com/notifications/unsubscribe-auth/ADJvulHniXVbRdOwTNyd1kpYyIRzEF4vks5sdDQ_gaJpZM4O7pld .