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

Fully Integrated Ace Editor/Viewer #22

Merged
merged 0 commits into from Aug 13, 2016
Merged

Conversation

@JayWood
Copy link
Contributor

@JayWood JayWood commented Apr 11, 2016

@jtsternberg - Cleaned up the ancient code a bit, please take a look and compare/test. As always with any questions let me know.

@jtsternberg
Copy link
Owner

@jtsternberg jtsternberg commented Apr 11, 2016

One problem I see here off-the-bat is that current users may be pretty peaved if we change the render engine out from under them. I think Ace is prob. the way to go, but we need to make sure the front-end rendering does not change unless they opt into it (and ace could be the default for new installs).

@JayWood
Copy link
Contributor Author

@JayWood JayWood commented Apr 11, 2016

@jtsternberg good point, didn't think about that, just wanted to update the old PR from 2015 haha - I'll get to work on the front-end to prevent an angry mob 👍

@JayWood
Copy link
Contributor Author

@JayWood JayWood commented Apr 12, 2016

@jtsternberg Nice and tidy, cleaned up a few things as well - I'll patiently await your response 😄

Ended up splitting up the two shortcode outputs into helper methods.

I didn't really know of a way to 'enable ace for new installs' aside from checking against the installed version. In the interest of simplicity, I just went with a filter to enable/disable the front-end shortcode ACE output.

So, to enable the ACE shortcode output, one only has to drop this somewhere in their theme/plugin:
add_filters( 'snippets-cpt-ace-frontend', '__return_true' );

@bradp
Copy link

@bradp bradp commented Apr 12, 2016

If you're bundling ACE, make sure you keep the License intact, as you'll want to properly credit the author and uphold the terms of the License.

@JayWood
Copy link
Contributor Author

@JayWood JayWood commented Apr 12, 2016

Noted!

@bradp
Copy link

@bradp bradp commented Jul 19, 2016

@jtsternberg yoooooooo

@jtsternberg
Copy link
Owner

@jtsternberg jtsternberg commented Aug 4, 2016

I'm soooo on this... soon.


if ( ! is_wp_error( $terms ) ) {
foreach ( $terms as $term ) {
wp_delete_term( $term->term_id, 'languages' );
Copy link
Owner

@jtsternberg jtsternberg Aug 7, 2016

Choose a reason for hiding this comment

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

Why are you deleting existing terms, and not providing an update path? Won't this remove the previously-applied language associations? (e.g. I have a "php" snippet, I update, the snippet is no longer set to "php")

Copy link
Owner

@jtsternberg jtsternberg Aug 7, 2016

Choose a reason for hiding this comment

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

Also of note, this wp_delete_term never actually deleted anything because $term is an integer, so there is no $term->term_id.

Copy link
Contributor Author

@JayWood JayWood Aug 8, 2016

Choose a reason for hiding this comment

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

@jtsternberg it's been so long, I honestly have no idea at this point.

Copy link
Owner

@jtsternberg jtsternberg Aug 8, 2016

Choose a reason for hiding this comment

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

Ok, no problem. I've made the appropriate changes. This is a big update, so there's lots of review.

Copy link
Contributor Author

@JayWood JayWood Aug 8, 2016

Choose a reason for hiding this comment

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

@jtsternberg - purely an excuse, but during the time of writing this I was using sublime, so it didn't complain about much - if I were using PHPStorm this wouldn't have happened 😄

Don't judge me lol

@jtsternberg jtsternberg merged commit f79ab60 into jtsternberg:master Aug 13, 2016
@jtsternberg
Copy link
Owner

@jtsternberg jtsternberg commented Aug 13, 2016

Thanks for all your effort @JayWood... Finally merged! I've made a lot of changes, so I suggest reviewing master for changes if you want. I'll be releasing an official version shortly (on wordpress.org).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants