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 Formless component template file #53

Merged
merged 9 commits into from Aug 19, 2019

Conversation

@JordanMartinez
Copy link

commented Aug 10, 2019

What does this pull request do?

Fixes #49

Also changes halogen-storybook's branch from halogen5 to master as the code wouldn't compile otherwise.

Where should the reviewer start?

Read the 'other notes' section, skim the code and then answer my questions below

How should this be manually tested?

AFAIK, we should only verify that this code compiles in the CI.

Other Notes:

There's a number of things that aren't correct in this PR with regard to the data type, Name.

  • the output field of the row type isn't Name as it should be but String
  • there might be a few type classes that Name unnecessarily implements (can't remember)
  • there isn't an optic for FieldError, so the usage of _Error will likely stop compilation
  • there's no example of the Initial type class and how one would could implement it for Name

The halogen component part is basically the uncommented version of the template file. Since it's also lacking in a few other ways (more modular pieces that show how to use some of its API?), it would need to be completed before we could document it better. I'm not sure what should be included here and what should be left out because the examples folder already exists.

Questions I have:

  • What else is st.form used for?
  • I think the commented version should include 'additional state' as an example of its usage whereas the uncommented one should not. Your thoughts?
  • I think this file should be separately licensed to ensure that one who uses it (since that's what it's for, right?) don't have to worry about satisfying such license constraints. Your thoughts?

Lastly, there's documenting each part in a clear manner.

@JordanMartinez JordanMartinez force-pushed the JordanMartinez:componentFile branch from 47f8ad7 to b0eea59 Aug 13, 2019

@JordanMartinez

This comment has been minimized.

Copy link
Author

commented Aug 13, 2019

@thomashoneyman Mind taking a look at this sometime soon so I can finish it up?

@thomashoneyman thomashoneyman self-requested a review Aug 17, 2019

@thomashoneyman thomashoneyman self-assigned this Aug 17, 2019

import Template.DataAndValidation (NAME_FIELD, _name, minLength)
import Type.Row (type (+))

type FormFieldsRow f =

This comment has been minimized.

Copy link
@thomashoneyman

thomashoneyman Aug 17, 2019

Owner

I quite like this!

, initialize = Just Initialize
, finalize = Just Finalize
, receive = Just <<< Receive
}

This comment has been minimized.

Copy link
@thomashoneyman

thomashoneyman Aug 17, 2019

Owner

I think each of these (mkInput included) could use a one-line description of what it's for, just as a reminder. If you would like, I can add those in for you, or feel free to do so.

Something simple like:

-- Decide what, if anything, to do when Formless events occur. If you would like to raise 
-- events as messages, then use `F.raiseResult` as your `handleEvent` function.
handleEvent :: ...
[ HP.type_ InputText
, HP.placeholder "Michael"
, HP.value (F.getInput _name st.form) -- access one value in form
, HE.onValueInput (Just <<< F.setValidate _name)

This comment has been minimized.

Copy link
@thomashoneyman

thomashoneyman Aug 17, 2019

Owner

Perhaps a comment here, too:

Suggested change
, HE.onValueInput (Just <<< F.setValidate _name)
, HE.onValueInput (Just <<< F.setValidate _name) -- set the value of _name, then validate it
@thomashoneyman

This comment has been minimized.

Copy link
Owner

commented Aug 17, 2019

Thanks for adding this @JordanMartinez -- I'm happy to merge with a couple of small tweaks I mentioned above. This is quite nice. I'll debug the CI failure, but it's not to do with your PR and so I am happy to merge even if I haven't quite fixed that yet.

@JordanMartinez

This comment has been minimized.

Copy link
Author

commented Aug 18, 2019

I hope to get to this sometime tonight.

@JordanMartinez

This comment has been minimized.

Copy link
Author

commented Aug 18, 2019

@thomashoneyman I've addressed all your comments except for documenting more parts. I think that might be better handled by you since you're more familiar with this library than I am.

-> F.ComponentHTML Form Action ChildSlots m
render st =
HH.div_
[ HH.p_ [ HH.text $ "Validity: " <> show st.validity ]

This comment has been minimized.

Copy link
@JordanMartinez

JordanMartinez Aug 18, 2019

Author

Indicates whether the form's values are valid (i.e. validation has passed for all fields)

render st =
HH.div_
[ HH.p_ [ HH.text $ "Validity: " <> show st.validity ]
, HH.p_ [ HH.text $ "Dirty: " <> show st.dirty ]

This comment has been minimized.

Copy link
@JordanMartinez

JordanMartinez Aug 18, 2019

Author

Indicates whether the form is currently being modified/validated.

This comment has been minimized.

Copy link
@thomashoneyman

thomashoneyman Aug 19, 2019

Owner

Indicates whether any field in the form has been changed from its initial state

HH.div_
[ HH.p_ [ HH.text $ "Validity: " <> show st.validity ]
, HH.p_ [ HH.text $ "Dirty: " <> show st.dirty ]
, HH.p_ [ HH.text $ "Being Submitted: " <> show st.submitting ]

This comment has been minimized.

Copy link
@JordanMartinez

JordanMartinez Aug 18, 2019

Author

Indicates whether the 'submit' button has been clicked and the form's content is still being validated one last time before submission is accepted.

[ HH.p_ [ HH.text $ "Validity: " <> show st.validity ]
, HH.p_ [ HH.text $ "Dirty: " <> show st.dirty ]
, HH.p_ [ HH.text $ "Being Submitted: " <> show st.submitting ]
, HH.p_ [ HH.text $ "Number of Errors: " <> show st.errors ]

This comment has been minimized.

Copy link
@JordanMartinez

JordanMartinez Aug 18, 2019

Author

Indicates the number of errors due to validation failing that need to be fixed before submission is accepted

, HH.p_ [ HH.text $ "Dirty: " <> show st.dirty ]
, HH.p_ [ HH.text $ "Being Submitted: " <> show st.submitting ]
, HH.p_ [ HH.text $ "Number of Errors: " <> show st.errors ]
, HH.p_ [ HH.text $ "Number of Submit attempts: " <> show st.submitAttempts ]

This comment has been minimized.

Copy link
@JordanMartinez

JordanMartinez Aug 18, 2019

Author

Indicates the number of times user has attempted to submit the form

, HH.p_ [ HH.text $ "Being Submitted: " <> show st.submitting ]
, HH.p_ [ HH.text $ "Number of Errors: " <> show st.errors ]
, HH.p_ [ HH.text $ "Number of Submit attempts: " <> show st.submitAttempts ]
, HH.p_ [ HH.text $ "Additional state was: " <> show st.additionalState ]

This comment has been minimized.

Copy link
@JordanMartinez

JordanMartinez Aug 18, 2019

Author

This is how we refer to the additional labels we added to the form's state in AddedState.

This comment has been minimized.

Copy link
@thomashoneyman

thomashoneyman Aug 19, 2019

Owner

We can also refer to any additional labels we used to extend the form's state; in this case, that means any field from our AddedState type.

:: F.Event Form AddedState
-> F.HalogenM Form AddedState Action ChildSlots Message m Unit
handleEvent = case _ of
F.Submitted formContent -> do

This comment has been minimized.

Copy link
@JordanMartinez

JordanMartinez Aug 18, 2019

Author

Indicates that the form has been successfully submitted.


-- This line exists so the code compiles.
pure unit
F.Changed formState -> do

This comment has been minimized.

Copy link
@JordanMartinez

JordanMartinez Aug 18, 2019

Author

Indicates that the form's content has been changed. This event is triggered anytime a field is changed, whether it passes validation or not.

@JordanMartinez
Copy link
Author

left a comment

I'm not sure what you had in mind for adding comments to each part, but this is what I would put. Feel free to correct/revise, etc. and I'll update it to what we agree upon here and push the changes following that.

@thomashoneyman

This comment has been minimized.

Copy link
Owner

commented Aug 19, 2019

I've added a thumbs-up to your suggestions which I think are good as-is, and I've added my own version for ones that I'd tweak. On the whole you're almost spot-on with what I'd write. Thanks!

@thomashoneyman

This comment has been minimized.

Copy link
Owner

commented Aug 19, 2019

CI has been fixed thanks to @linearray in #54, so once your updates here are applied I'll merge this in. Thanks again!

@JordanMartinez

This comment has been minimized.

Copy link
Author

commented Aug 19, 2019

Done! Just waiting on CI now.

@JordanMartinez JordanMartinez force-pushed the JordanMartinez:componentFile branch from b45d1a7 to bb1b286 Aug 19, 2019

@thomashoneyman

This comment has been minimized.

Copy link
Owner

commented Aug 19, 2019

@JordanMartinez mind rebasing or merging the current halogen5 branch into yours to get the CI passing? Thanks!

@thomashoneyman

This comment has been minimized.

Copy link
Owner

commented Aug 19, 2019

Actually -- don't worry about it, I'll just merge and let things run there. Thanks again!

@thomashoneyman thomashoneyman merged commit c3ded20 into thomashoneyman:halogen-5 Aug 19, 2019

1 check failed

ci/circleci: test Your tests failed on CircleCI
Details
thomashoneyman added a commit that referenced this pull request Aug 19, 2019
Update Formless to be Halogen 5 compatible (#46)
* initial commit for v5 -- library compiling

* tweaks

* rework to support queries and actions

* update app helpers to latest Select

* update most examples (todo: nested-array, real-world)

* update nested array example

* switch queries to allow access to public actions

* examples building -- missing querying only

* complete real world example

* clean up real world example

* update readme

* tweak readme

* fix demo examples

* add warning to readme about versions

* allow users to provide  to generate initial form values

* tweak readme

* switch to Maybe for initial inputs fields

* move internal functions from component to Internal module and add message to Slot' type

* add raiseResult function to reduce boilerplate; adjust Slot' type to accept a message

* update Slot' type

* fix type signature of raiseResult

* Remove extra whitespace

* Change type parameter name: 'ps' (Halogen 4) to 'slots' (Halogen 5)

* Expose Halogen's input type

* Update examples to account for Halogen's `input` type

* Renamed modules: 'Component' to 'Page' and 'FormSpec' to 'Form'

* Remove unneeded `defaultEval`: already creating the full record

* Rename Formless' `Message` type to `Event`

* Update examples to use new `Event` type

* Import & whitespace fixes for PureScript 0.13 (#51)

* Fix mixed indentation
* Update Row and RowList imports

* Make examples compile again. (#54)

* Add Formless component template file (#53)

* Added Formless component template

* Further cleanup template

* Fix indentation for template

* Fix typo in template example

* Template file: convert MonadType to m with MonadAff constraint

* Template file: document handleEvent function

* Temlate file: document mkInput

* Template file: more clearly document getInput and setValidate functions

* Use comments to further document template file
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.