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

Add support for rendering flash messages in templates #409

Closed
wants to merge 1 commit into from

Conversation

gabiudrescu
Copy link
Contributor

I have added flash notification in templates. looks like this for me:
image

in code, I generate them like this:

class AdminController extends BaseAdminController
{
    public function extendAction()
    {
        $em = $this->getDoctrine()->getManager();
        $repository = $this->getDoctrine()->getRepository("GabiUJobeetBundle:Job");

        $id = $this->request->query->get('id');

        $entity = $repository->find($id);
        $entity->extend();

        $em->persist($entity);
        $em->flush();

        $this->addFlash("success", "Entity updated");

        return $this->redirectToRoute('admin', array(
            "view" => 'edit',
            "entity" => $this->request->query->get('entity')
        ));
    }
}

@javiereguiluz
Copy link
Collaborator

@gabiudrescu thanks for proposing this change. To be honest, I prefer to not add this feature. Why? Because I believe notifications should be made when things go wrong, not when things worked (which by the way, it's 99.99% of the times). If you take a look at well-designed desktop applications, they never notify you when things go right.

@Pierstoval
Copy link
Contributor

I'm also 👎 for this feature, even if it's well implemented. But we should provide a small cookbook when customers want this kind of feature in backends "we" develop.

@javiereguiluz
Copy link
Collaborator

@Pierstoval yes, that's why I haven't closed this issue. I don't want it in the default bundle ... but I want to make it easy to use it for people who really want that feature.

@gabiudrescu
Copy link
Contributor Author

hi Javier,

the PHP code above is my own code, in my own bundle. The pull request is about the ability of rendering any kind of notifications.

for example:
image

@Pierstoval
Copy link
Contributor

@gabiudrescu I think we totally got it but actually these notifications depend on an app-specific issue.
I have more than 5 projects using EasyAdmin, and none of them need any notification like these. It's subjective, of course, but it's actually a 100% non-use for this behavior.

But as you did in this PR, implementing it is really really easy, so it's not a problem.

@gabiudrescu
Copy link
Contributor Author

yeah, indeed, the code should have hinted me it's self explanatory. anyway, thanks for your feedback.

I'd be curious - how do you notify users in case something bad happens?

@Pierstoval
Copy link
Contributor

If something bad happens, it should throw an exception (it's the best practice) so they're handled by EasyAdmin IIRC.

@javiereguiluz
Copy link
Collaborator

Yes. When some error happens, we throw an exception to display a proper error page. Of course we still need to add more error pages, but we've already covered some of the biggest problems: https://github.com/javiereguiluz/EasyAdminBundle/tree/master/Exception

@gabiudrescu
Copy link
Contributor Author

thanks everyone for the feedback. I'd like to use my modifications in this pull request in an application I'm currently developing. any tips and tricks about how I can automatically do that and still follow the master branch development (when there's any new addition)?

@Pierstoval
Copy link
Contributor

Just override the layouts with the EasyAdmin override system, extend the original layouts, and customize the part you need:

{% block content_header %}
    {{ include('easy_admin/custom_notification_template.html.twig') }}
    {{ parent() }}
{% endblock %}

@javiereguiluz javiereguiluz changed the title adding flash notification support Add support for rendering flash messages in templates Aug 31, 2015
@javiereguiluz
Copy link
Collaborator

@gabiudrescu I'm finishing this feature in #430. That's why I'm closing this pull request. Obviously I've reused your original commit, so you'll get full credit for your work. Thanks!

javiereguiluz added a commit that referenced this pull request Sep 3, 2015
…iluz)

This PR was merged into the master branch.

Discussion
----------

Added support for flash messages

This finishes #409.

Although EasyAdmin won't display flash messages, it now supports this feature to simplify the work of the developers who use these messages.

This adds a new template called `flash_messages` which can be overridden following the usual mechanism available for the other templates. The HTML markup of the default template is enough to display the messages correctly, being one or several:

![one-flash-message](https://cloud.githubusercontent.com/assets/73419/9643111/36d2f1ca-51c0-11e5-8208-38b141291237.png)

![several-flash-messages](https://cloud.githubusercontent.com/assets/73419/9643112/379a69ee-51c0-11e5-8560-d5bf9431d534.png)

Commits
-------

8b8186e Deduplicate code
9fb4c04 Define the flash_messages block in the containing templates
a92239a Added support for Flash Messages
24d7db2 adding flash notification support
@gabiudrescu
Copy link
Contributor Author

whohoo, that's good news 👍 though your modifications in feature #430 have a much cleaner implementaton, I'm glad my idea has been useful for such an awesome bundle.

keep up the good work!

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

Successfully merging this pull request may close these issues.

None yet

3 participants