feat(elm): Introduce form validation library #63

pmrukot · 2017-07-22T21:17:33Z

Summary
Introduces form client form validation.

Related issues
#61

pmrukot · 2017-07-23T18:28:26Z

Update: Added advanced validators with regex, form is being cleared upon successful submission.
Would love to add Toast notifications if the submission was successful or not, but waiting for:
#59

mrapacz · 2017-07-26T21:54:19Z

aion/web/elm/src/Update.elm

@@ -27,7 +28,25 @@ update msg model =
            { model | user = response } ! []

        OnQuestionCreated response ->
-            model ! []
+            case response of
+                RemoteData.Success responseData ->


Actually I'm not sure what's the point of using RemoteData since we are ignoring all of its privileges. The main cause for using it is the fact that we can distinguish between having loaded empty or not yet having loaded data etc. Seeing how we deal with this everytime makes me cringe and rethink if we should really use it at all if that's the way we're using it. I mean, we can always say that we will implement rest of the pattern checks later, but we both know it's one of the lowest priority things.

I think we should use RemoteData. Maybe this particular piece of code is not the best example but the abstraction layer it adds is actually nice. As a developer you don't need to worry about all the edge cases that can be returned from the server.

mrapacz · 2017-07-26T21:57:32Z

aion/web/elm/src/Update.elm

+
+                validationErrors =
+                    possibleFields
+                        |> List.map (\name -> Forms.errorList model.panelData.createQuestionForm name)


For me it works as the Elixir's style of aliasing nested modules - I would put let createQuestionForm = model.panelData.createQuestionForm to get it of this dot army.

mrapacz · 2017-07-26T22:00:14Z

aion/web/elm/src/Update.elm

+                        newCreateQuestionForm =
+                            Forms.updateFormInput oldCreateQuestionForm "question" ""
+
+                        evenNewerCreateQuestionForm =


Haven't checked that, but instead of these three createQuestionForm lets we could do with one using pipe chains. How about createQuestionForm -> updateFormInput -> updateFormInput?

Basically if you define bindings in let area only to use them in the next binding in the same let area, that suggests that you could actually just go ahead and use a pipechain.

Im not sure how to handle it in nice way 😑

mrapacz · 2017-07-26T22:04:22Z

aion/web/elm/src/Update.elm

-                oldPanelData =
-                    model.panelData
+                possibleFields =
+                    [ "question", "answers", "subject" ]


I totally wouldn't hardcode these (definitely not in Update.elm, I say we stick to only having logic here, not data). Maybe we could find a cozy place for constants like that? We could put it in appropriate Models.elm or Constants.elm file.

mrapacz · 2017-07-26T22:09:10Z

aion/web/elm/src/Panel/View.elm

+    div []
+        [ p [] [ text "Enter answer below, separate all posibilities with comma:" ]
+        , input
+            [ placeholder "4,cztery"


Polish -> English

I think the views could be split to more functions, but maybe I'll do it when #56

mrapacz elmrefactor when

mrapacz · 2017-07-26T22:14:32Z

aion/web/elm/src/Panel/View.elm

 listRooms : WebData RoomsData -> List String
 listRooms result =
    case result of
        RemoteData.Success roomsData ->
-            List.map (\room -> room.name) roomsData.data
+            "" :: List.map (\room -> room.name) roomsData.data


Okay, the "":: part is impressive and illegible. What does it actually do? I'm sure we can come up with a better way of solving this problem

Validation library does not provide default values. So this is a dirty hack to have empty string at the beginning to force user to choose something. If the user would like to create a new question to the first displayed category he would need to choose something else and get back to it and then hit submit.

Maybe we need better solution for a dropdown menu though.

mrapacz · 2017-07-26T22:16:19Z

aion/web/elm/src/Update.elm

-                { model | panelData = { oldPanelData | newQuestionContent = questionContent } } ! []
+                { model
+                    | panelData =
+                        { oldPanelData | createQuestionForm = Forms.updateFormInput oldPanelData.createQuestionForm name value }


Same here, you get rid of the dots using improved imports or cool variable bindings in let area just above and you're left with createQuestionForm = updateFormInput createQuestionForm name value. Seems more legible. I also think the naming could be better here. What if we changed createQuestionForm to questionForm? The create verb confuses me a lot as I imagine it is a name of a function that actually creates a questionForm

I think we should leave Forms prefix so we know where this function comes from.

mrapacz · 2017-07-26T22:18:33Z

aion/web/elm/src/Update.elm

+                if List.isEmpty validationErrors then
+                    ( model, createQuestionWithAnswers model )
+                else
+                    model ! []


What happens if there actually are errors? (serious question, what do we want to do, react somehow - flash info message or just leave it and let user figure out why it's not working?)

I was thinking about displaying Toasties but Im still waiting for #59 to be merged.

mrapacz · 2017-07-26T22:21:08Z

aion/web/elm/src/Update.elm

-            in
-                { model | panelData = { oldPanelData | newAnswerCategory = answerCategoryToId } } ! []
+                if List.isEmpty validationErrors then
+                    ( model, createQuestionWithAnswers model )


I think it's a matter or #56 but I wouldn't pass the whole model to any function (apart from main Update etc). I believe that we should always only pass one data structure from our model. Well, what if I would then need to pass like three different structures from our model, eh? one could ask. I believe that if we split our state well, these all three different structures should end up in one bigger structure that aggregates them.

mrapacz

@pmrukot Bring ye the whole answers into the discussion.

pmrukot · 2017-08-12T15:34:12Z

@mrapacz I've updated according to your suggestions, only not sure how to handle pipe chains. For me It would be also good to merge it and adjust later since we need to move forward.

mrapacz

Looks acceptable imho, looks really neat. Let's leave it like this for now, I may fix the small things while doing the elm refactor.

pmrukot added Status: Work in progress Type: Enhancement labels Jul 22, 2017

mrapacz reviewed Jul 26, 2017

View reviewed changes

pmrukot added 7 commits August 12, 2017 13:06

feat: Introduce form validation library, refactor

c084fda

feat(elm): Add advanced validators

a4b1879

feat(elm): Clear form upon succesful submission

e572c1e

fix: Make code compile

257a8fd

fix: Better regex for answers

0570481

fix: Dont make request if there are validation errors

1aa2014

fix: Review adjustments

fcdb031

pmrukot force-pushed the feat/multiple-asnwers-support-client branch from 4da66e7 to fcdb031 Compare August 12, 2017 15:31

pmrukot added Status: Completed and removed Status: Work in progress labels Aug 12, 2017

mrapacz approved these changes Aug 14, 2017

View reviewed changes

mrapacz removed the Status: Completed label Aug 14, 2017

pmrukot added 2 commits August 15, 2017 09:55

Merge branch 'master' into feat/multiple-asnwers-support-client

6bb179c

fix: Update mix.lock

a167fcc

pmrukot merged commit 526fdde into master Aug 15, 2017

pmrukot deleted the feat/multiple-asnwers-support-client branch August 15, 2017 08:05

mrapacz pushed a commit that referenced this pull request Aug 15, 2017

feat(elm): Introduce form validation library (#63)

02dc294

mrapacz pushed a commit that referenced this pull request Aug 15, 2017

feat(elm): Introduce form validation library (#63)

bbb1b2c

mrapacz pushed a commit that referenced this pull request Aug 16, 2017

feat(elm): Introduce form validation library (#63)

be14c7b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(elm): Introduce form validation library #63

feat(elm): Introduce form validation library #63

pmrukot commented Jul 22, 2017

pmrukot commented Jul 23, 2017

mrapacz Jul 26, 2017

pmrukot Jul 27, 2017

mrapacz Jul 26, 2017

mrapacz Jul 26, 2017 •

edited

Loading

mrapacz Jul 26, 2017

pmrukot Aug 12, 2017

mrapacz Jul 26, 2017

mrapacz Jul 26, 2017

mrapacz Jul 26, 2017

mrapacz Jul 26, 2017 •

edited

Loading

mrapacz Jul 26, 2017

pmrukot Jul 27, 2017

pmrukot Jul 27, 2017

mrapacz Jul 26, 2017

pmrukot Aug 12, 2017

mrapacz Jul 26, 2017

pmrukot Jul 27, 2017

mrapacz Jul 26, 2017

mrapacz left a comment

pmrukot commented Aug 12, 2017

mrapacz left a comment

feat(elm): Introduce form validation library #63

feat(elm): Introduce form validation library #63

Conversation

pmrukot commented Jul 22, 2017

pmrukot commented Jul 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrapacz Jul 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrapacz Jul 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrapacz left a comment

Choose a reason for hiding this comment

pmrukot commented Aug 12, 2017

mrapacz left a comment

Choose a reason for hiding this comment

mrapacz Jul 26, 2017 •

edited

Loading

mrapacz Jul 26, 2017 •

edited

Loading