Fix state bugs - Githubissues

wp-shortcake / shortcake

Shortcake makes using WordPress shortcodes a piece of cake.

GNU General Public License v2.0

665 stars 143 forks source link

Fix state bugs #728

Closed mattheu closed 7 years ago

mattheu commented 7 years ago

I ran into a couple of bugs with the modal state.

The first is that the media library / upload tabs show when opening the modal for a second time. This bug has been present since before the last release.

Steps to reproduce:

Click add post element button
Close modal with X in top right
Click add post element button again

As can be seen in the screenshot, these tabs should not be visible.

The second issue is that after editing an exsisting shortcode, the state is not correctly reset on close.

Steps to reproduce.

Either insert a shortcode from shortcake or find one that exists.
Click the edit button
Click the cross in the top right of the modal
Click 'add post element' button

You see the following screen. I expect to see the grid of shortcodes I can insert. The media frame state was not correctly reset on close.

goldenapples commented 7 years ago

This fixes both of the bugs noted. I hadn't even noticed the router showing up on the shortcode frame, but that has been an issue for a long time now... thanks!

The only question I have, in looking over the code, is whether there's any reason that MediaController.reset() and MediaController.resetState() need to be kept as separate methods. It looks like with one exception, whenever we call one, we're also calling the other one. And here, where we're setting the media controller before rendering the insert frame of the window, it would make just as much sense to reset the state as well.

If it makes sense to keep the two actions separated, that's totally fine. I'm just wondering if they can be combined into one method for simplicity though.

mattheu commented 7 years ago

The only question I have, in looking over the code, is whether there's any reason that MediaController.reset() and MediaController.resetState() need to be kept as separate methods.

Agreed. Not sure why I did it like that :)

mattheu commented 7 years ago

@goldenapples Updated this to consolidate the 2 methods.

goldenapples commented 7 years ago

Wait a sec - I spoke too soon. It looks like the specs.js file isn't updated in the last build commit. I'm not sure why that is. If you can rebuild it here, that'd be great. Otherwise, we can just build it on master...

mattheu commented 7 years ago

specs.js file isn't updated

This file is a little out of date and I see a few changes unrelated to this change. I will open a new PR to recompile everything as I"m also seeing CSS changes.

goldenapples commented 7 years ago

@mattheu Can you merge in the latest master and finish off this PR, so we can push this fix in a new patch release? If you're busy right now, I can finish it up for you... just let me know.

mattheu commented 7 years ago

Will do. I had forgotten this one wasn't merged.

mattheu commented 7 years ago

@goldenapples Merged with master - ready to go.

goldenapples commented 7 years ago

Nice. I checked in the updated specs file to make sure that tests still pass - they do. I'll merge this now.