-
Notifications
You must be signed in to change notification settings - Fork 0
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
feat(elm): Display notification If creating a question was successful #74
Conversation
aion/web/elm/src/Panel/View.elm
Outdated
|
||
|
||
panelView : Model -> Html Msg | ||
panelView model = | ||
div [] | ||
[ h3 [] [ text "Create new question for certain category:" ] | ||
, form [] | ||
, form [ onWithOptions "submit" { preventDefault = True, stopPropagation = False } (Json.Decode.succeed (NoOperation)) ] |
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.
Wow, I dont need this, will remove. 🤕
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.
True, nice catch 😂
|
||
questionCreationSuccessfulToast : ( Model, Cmd Msg ) -> ( Model, Cmd Msg ) | ||
questionCreationSuccessfulToast = | ||
addToast (Toasty.Defaults.Success "Success!" "Question created successfully.") |
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.
Should this file be moved into General/Notifications? Or do we want to have it per resource. If yes I need to move it to separate file. (Not a big deal) @mrapacz any opinion?
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.
I have no idea yet. I suggested my approach to that one, I think it would be cool to have Notifications.elm file containing only notification messages and have different notification files in certain resources.
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.
I've already did that here: #76, will apply necessary changes to this PR and then rebase the one above. My feedback to your suggestion -> Basically it all comes down to taking addToasts
function higher and storing the message strings in some way. But this level of verbosity that we currently have is not necessarily bad, I think we can live with it for mow.
-- question creation | ||
|
||
|
||
questionFormValidationErrorToast : ( Model, Cmd Msg ) -> ( Model, Cmd Msg ) |
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.
I feel like we're repeating ourselves a lot with these toasty notifications.
I think a cooler approach would be to just create three types of Toasty notifications and define error messages in either Errors.elm or Constants.elm (although I would opt for the first one) and just call the function like ErrorToast questionFormValidationError.
What do you think? We do not need to necessarily introduce it just now as we have big Elm refactor coming anyway.
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.
I think I've already answered to that above. Refactor can come later when our Panel section will be in more advanced stage, maybe we will be able to see more patterns to refactor. You know, premature optimisation blah blah...
@@ -45,10 +45,10 @@ update msg model = | |||
evenNewerQuestionForm = | |||
Forms.updateFormInput newQuestionForm "answers" "" | |||
in | |||
{ model | panelData = { oldPanelData | questionForm = evenNewerQuestionForm } } ! [] | |||
questionCreationSuccessfulToast ({ model | panelData = { oldPanelData | questionForm = evenNewerQuestionForm } } ! []) |
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.
We'll also need to create cool setters, but that's also coming with #56.
Summary
Related issues
Closes #61
Closes #72
Test plan
Just play around question form submission and see if notifications are being displayed.