-
-
Notifications
You must be signed in to change notification settings - Fork 573
Conversation
@apaganobeleno Should we add a |
at least success.
…On Wed, Jan 18, 2017 at 9:24 PM, Brian Scott ***@***.***> wrote:
@apaganobeleno <https://github.com/apaganobeleno>
Should we add a notice and/or success message level?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#143 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AACSdDT6tD0tH2mTu2qwmy5xeqt2dwOlks5rTsligaJpZM4LnlnE>
.
--
----
Brian Ketelsen
bketelsen <at> gmail <dot> com
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's looking good. A few comments. We should be able to add multiple messages to a key, so there should be an 'Add' function as well as a 'Set'
I also wonder if instead of having the session in the flash strict, what if we saved the flash to the session at the end of the request, instead of everytime we need to get or set things. It might be easier and cleaner.
Also, in the view we need to be able to check for the existence of a flash before we render code to display it. If I'm reading the code correctly the 'defer' call would delete it if we did an existence check before we render.
That all make sense? It's looking good though.
@markbates great, i'll work in those changes and commit as i have these, i like the idea of loading and then storing into session at the end, thanks for that one. |
so much excite! |
@markbates when checking the existence of a flash var, should we provide a helper just for that (something like @bscott i'm very close, to have it :D #suspense. |
Nice! I'm assuming flash will work for validation errors in models as in the Rails world? |
@markbates oh also, when you call |
Ok, So I've given this some more thought. :) I think we've been over thinking things a bit. Since flash is basically a wrapper around {{#each flash.Errors as |e|}}
<div class="alert alert-danger" role="alert">{{e}}</div>
{{/each}} That is all the template needs. From the Go side it would look something like this: type Flash struct {
errors []string
}
func (f *Flash) Errors() []string {
defer func() { f.errors = []string{} }()
return f.errors
} After With this approach, we don't really need a The Does that make sense? Also, that code above works if you have a Handler that looks like this: func FlashMe(c buffalo.Context) error {
flash := &Flash{
errors: []string{"John", "Paul", "George", "Ringo"},
}
c.Set("flash", flash)
return c.Render(200, r.HTML("flasher.html"))
} |
…ded view helpers
Oh boy! thats way simpler and cleaner, just updated to that you suggested. it works with: {{#each flash.Errors as |error|}}
{{error}}
{{else}}
Nothing Found
{{/each}} and {{#each flash.All as |k flash|}}
{{k}}:{{flash}}
{{else}}
No Flash here!
{{/each}} |
@markbates should we add Notice and Success as well ? |
Can you add some tests that shows usage in the template? I know that I can iterate using the |
I think we are almost there with this! |
@@ -102,6 +108,11 @@ func (d *DefaultContext) Render(status int, rr render.Renderer) error { | |||
data["params"] = pp | |||
bb := &bytes.Buffer{} | |||
err := rr.Render(bb, data) | |||
|
|||
if d.Flash() != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be moved after the err
check from rr.Render
.
@markbates do you think |
Unfortunately If we change |
I don't think we need predefined levels. Let's keep the code small for now. I, however, like the idea of purging the flash after a render. If you don't use it, you loose it. I can't think of a use case, other than redirect, where someone would need to persist a flash across many pages before displaying it. |
@markbates updated!, now flash for the view is |
@markbates something else to add/change here ? |
@apaganobeleno I'm in a class now. When I get back to the hotel I'all pull it down and play with it |
@markbates Thank you sir! |
Thank you! I can't wait to try it out! |
|
||
//Persist the flash inside the session. | ||
func (f *Flash) Persist(session *Session) { | ||
for k := range session.Session.Values { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can clean all of this up by marshaling the flash and put just one thing into the session. Perhaps using json.
|
||
if session.Session != nil { | ||
for k := range session.Session.Values { | ||
sessionName := k.(string) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use json to marshal the flash in and out of the session and this gets a lot cleaner.
@apaganobeleno I made a few changes to the PR. can you review? I think this is ready to merge. |
@markbates changes look great, thanks for that Mark. i noticed an error being thrown when the each is iterating on a non-set value of the flash: {{#each flash.errors as |k flash|}}
{{k}}:{{flash}}
{{else}}
Nothing Found
{{/each}} causes :
but {{#each flash as |k flash|}}
{{k}}:{{flash}}
{{else}}
Nothing Found
{{/each}} does not, i'm thinking this should be handled in velvet |
@markbates i tried the same by doing: //on my actions
func GetFlashHandler(c buffalo.Context) error {
c.Set("myvar", map[string][]string{})
return c.Render(200, r.HTML("show.html"))
} And on my view: All:
{{#each myvar.errors as |k dat|}}
{{k}}:{{dat}}
{{else}}
Nothing Found
{{/each}} And same happens, would you accept a PR on velvet for this ?, basically on https://github.com/gobuffalo/velvet/blob/master/each_helper.go#L17 we should first check for
thoughts ? |
So with the fix to velvet, do we feel good about merging this @apaganobeleno? |
Feeling good about it, lets merge it :) 🚀 🚀 |
Woot!! Yay!! |
Merged!! |
@markbates @bketelsen @bscott this is where i am on the
flash
helper, i'll continue tomorrow with the iterator implementation, look forward for your feedback.