Task solution from bold-lion #28
Conversation
|
Hi there, is this then ready to be reviewed or are you going to add something more to it? |
scrive-cat-bot
added
the
ready for review
|
🥷 🥷 You can review it as is, thank you.
|
There was a problem hiding this comment.
Thank you for taking up the challenge.
I left some notes and questions for the discussion. In general I like how the code is structured and even though I have never worked with elm-monocle, I find it quite readable at this scope of project. On the other hand it feels little bit too overengineered but that was probably because you wanted to show us your skills.
The UI is nicely accessible and I really appreciate the usage of radio instead of select. Also correct use of labels is a nice touch.
| type Answer | ||
| = Text String |
There was a problem hiding this comment.
I think this was meant to be extended with other types of fields, right?
There was a problem hiding this comment.
🥷 🥷 As I wrote on the Field type comment, this is more about how we store the Answer. Answer.Number for Int, Answer.No or Answer.Yes for Boolean for example.
| ifEmpty subject = | ||
| case subject of | ||
| Text value -> | ||
| value == "" |
There was a problem hiding this comment.
How would e.g. String.isEmpty be different? Why have you choose this approach?
There was a problem hiding this comment.
🥷 🥷 String.isEmpty, would be better yes, this one came to my mind first.
|
|
||
| hasMinLength : Int -> String -> Bool | ||
| hasMinLength minLength str = | ||
| String.length str >= minLength |
There was a problem hiding this comment.
This counts non-printable characters as well. I'd add some String.trim to the party
There was a problem hiding this comment.
🥷 🥷 Yes, good idea.
| type alias Option = | ||
| ( String, String ) |
There was a problem hiding this comment.
It is not visible what the Option actually is and what purpose it should serve. I think I'll find out later, but just now it is quite hard to imagine how it is going to be used
There was a problem hiding this comment.
🥷 🥷 It's for the value and label pair for Radio, and can be for Dropdown too.
| type Field | ||
| = Text TextType | ||
| | Radio (List Option) | ||
| | Checkbox |
There was a problem hiding this comment.
This looks quite like the variants in Data.Answer module. Is it that Data.Answer.Text is a counterpart for the Text here, but the others don't actually need the validations? 🤔
There was a problem hiding this comment.
🥷 🥷 Answer could evolve into types like Yes, No, Number, Selected etc. It is about how we store the answers. While these Field types are more about how we show them on UI.
|
|
||
|
|
||
| type alias Form = | ||
| ( Entries, ReplyValidation ) |
There was a problem hiding this comment.
Is the ReplyValidation here in the tuple tied to the whole form, while the others are tied to the fields, right? Isn't it kind of duplication of the ReplyValidation as you can get all of the Entries and get the ReplyValidation from the Entry?
There was a problem hiding this comment.
🥷 🥷 This ReplyValidation is more for the errors for the whole form instead of individual entries, for example the preferred empty contact method validation. When we check if the form has an error, we still check ReplyValidation of individual entries and the form specific ReplyValidation.
| Invalid _ -> | ||
| True |
There was a problem hiding this comment.
Does this stand for the variant of Invalid []? Would that be considered True? Is it possible to end up in that state?
There was a problem hiding this comment.
🥷 🥷 I used Invalid [] in validateEntries when question was not found. It could default to NotValidated instead. If we want to make this impossible, and want to make sure we always have an error string when it is Invalid, then I guess we could have first a string for the first element of the list and then the list like: Invalid String (List String)
| type alias ContactDetails = | ||
| { address : Address } |
There was a problem hiding this comment.
Isn't this kind of unnecessary step? I'd probably flatten the structure here.
There was a problem hiding this comment.
🥷 🥷 I thought it might later have other things within contact details.
| type alias Props = | ||
| { disabled : Bool } |
There was a problem hiding this comment.
Q: What is the benefit of having this as a single item record?
There was a problem hiding this comment.
🥷 🥷 Might be extended in future, better start with Props to reduce refactoring overload later imo.
Hello there.
I have implemented the address form part (that too partly) of this challenge.
I must admit I have spent more time than I should have on it, to make it in general more scalable. But perhaps it has become a bit more complicated than it could have been.
I added elm-review.
If I had more time, I would add unit tests, better UI (I used some generic styles from flowbite), better accessibility, and caching.
Also the inheritance does not work per se. When you deselect "Is Inherited?" checkbox and modify values and check it again, it does not reset the values.
It might have some other problems with validation, I could not battle-test it.
In any case, I enjoyed the challenge, and I hope you will find something insightful in it too.