Skip to content

Editor preview #528

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

Closed
wants to merge 7 commits into from
Closed

Editor preview #528

wants to merge 7 commits into from

Conversation

elkuku
Copy link
Contributor

@elkuku elkuku commented Apr 8, 2017

This will add an "editor" with preview functionality (as seen on GitHub 😉)

The "lesson" to learn here might be using the markdown service in the controller, making a simple AJAX request and respond with a JsonResponse object from the controller.

Please bear with me since I am very new to Symfony.

Currently the loading message ("Loading preview...") is hard coded inside the JavaScript file.
Does Symfony provide a better way to translate strings in JavaScript files or should I pass the string as a parameter?

Hope you find it useful 😉

@elkuku
Copy link
Contributor Author

elkuku commented Apr 9, 2017

So this is actually not working as expected.
There might be some additional Symfony lessons I have to learn.
Maybe something is missing in this empty class?

Sorry for the noise.. :(

@elkuku
Copy link
Contributor Author

elkuku commented Apr 9, 2017

OK I found the missing piece, even the unit tests are passing now :)

This seems to be a pitfall for inexperienced Symfony developers. Not sure what may have caused the misbehavior...

Copy link
Member

@yceruto yceruto left a comment

Choose a reason for hiding this comment

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

👍 for implement this nice feature :)

$text = $request->request->get('text');

if (!$text) {
$response['data'] = 'Nothing to preview...';
Copy link
Member

Choose a reason for hiding this comment

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

IMO should we return an empty string in this case.

$response['data'] = $this->get('markdown')->toHtml($text);
}

return new JsonResponse($response);
Copy link
Member

Choose a reason for hiding this comment

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

You can use $this->json() method to simplify this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool

web/js/main.js Outdated
preview: function (text, preview, previewUrl) {
var out = $(preview);

out.html('Loading preview...');
Copy link
Member

@yceruto yceruto Apr 9, 2017

Choose a reason for hiding this comment

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

To avoid the translation issue, you can set this text by default into tab panel preview and translate it then, or use an icon loader instead, in any case this line should be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a CSS loading animation to avoid the translations..

SymfonyDemo.preview('#post_content', '#previewId', '{{ path('preview') }}');
}
});
</script>
Copy link
Member

Choose a reason for hiding this comment

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

You can create a template file to do that and avoid duplicated code in edit.html.twig.

/**
* Converts a markdown string to HTML.
*
* @Route("/preview", name="preview")
Copy link
Member

Choose a reason for hiding this comment

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

You can name this route admin_post_content_preview to be consistent with this action and others routes?

*
* @author Nikolai Plath <[email protected]>
*/
class EditorType extends AbstractType
Copy link
Member

@yceruto yceruto Apr 9, 2017

Choose a reason for hiding this comment

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

If the only one value here is to override the block name used to render the form type, then use the block_name option instead (bonus plus to this PR) and remove entirely this form type.

For example:

// AppBundle\Form\PostType

->add('content', null, [
    // ...
    'block_name' => 'editor',
])
{# form/fields.html.twig #}

{% block _post_editor_widget %}
    ...
    {{ block('textarea_widget') }}
    ...
{% endblock %}

http://symfony.com/doc/current/reference/forms/types/form.html#block-name
http://symfony.com/doc/current/form/form_customization.html#form-customization-sidebar
http://symfony.com/doc/current/form/form_themes.html#form-template-blocks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Very cool 😜

@elkuku
Copy link
Contributor Author

elkuku commented Apr 9, 2017

@yceruto Thanks for the review. Everything makes perfect sense.
Thanks also for the links. Very much appreciated 😉

@@ -18,6 +18,7 @@
use Sensio\Bundle\FrameworkExtraBundle\Configuration\Security;
use Symfony\Bundle\FrameworkBundle\Controller\Controller;
use Symfony\Component\Form\Extension\Core\Type\SubmitType;
use Symfony\Component\HttpFoundation\JsonResponse;
Copy link
Contributor

Choose a reason for hiding this comment

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

@elkuku, Why do you import this class? You don't use it in the previewAction.

Copy link
Member

@yceruto yceruto left a comment

Choose a reason for hiding this comment

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

@elkuku Thanks to address all my comments quickly, let's wait to other opinions and the final approbation from members.

@@ -18,6 +18,7 @@
use Sensio\Bundle\FrameworkExtraBundle\Configuration\Security;
use Symfony\Bundle\FrameworkBundle\Controller\Controller;
use Symfony\Component\Form\Extension\Core\Type\SubmitType;
use Symfony\Component\HttpFoundation\JsonResponse;
Copy link
Member

Choose a reason for hiding this comment

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

unused import?

@elkuku
Copy link
Contributor Author

elkuku commented Apr 9, 2017

Thanks @voronkovich and @yceruto seems that your eyes are sharper than any of those automated testing tools :)

@elkuku
Copy link
Contributor Author

elkuku commented May 18, 2017

Is there anything else I should do here?
BTW.. test failures are not related to the code changes here (I hope) 😉

@elkuku
Copy link
Contributor Author

elkuku commented May 24, 2017

Sorry, but I just found #69 😉

@noniagriconomie
Copy link
Contributor

👍🏻 for this, 94noni is my personal account
And today i showed to someone how to deal with ajax

@javiereguiluz
Copy link
Member

I'm closing this because we already have an Ajax feature (see #613) and because this would complicate the application too much. Let's remember that we are not making something comparable to WordPress. The blog application is an excuse to show Symfony code. We don't acre about the blog system and we don't care if it doesn't have some features that you find in other CMS or blog systems.

@elkuku I'm REALLY sorry for closing this without merging it. I know it's awful to dedicate time to something that it's not merged. That's why I always recommend to open an issue first to discuss about the feature instead of opening a pull request directly. I hope you understand this decision. Thanks!

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.

5 participants