Task solution from happy-luna #16

kraklin · 2025-01-15T12:29:32Z

Ready for review

Contact details task
Settings task
Tags task

add default empty data implement preferred contact method input

implement reusable input

kraklin · 2025-01-15T12:38:28Z

🥷 Excuse me, but what is the deadline?

kraklin · 2025-01-15T12:38:33Z

🥷 Excuse me, but what is the deadline?

kraklin · 2025-01-15T13:58:44Z

🥷 Root user group can't use inheritance. - what does it mean? should it be empty fields if there is "inherited_from": "1" and "parent_id": null ? or it just should be editable fields in that case?

kraklin · 2025-01-15T21:01:31Z

🥷 Excuse me, but what is the deadline?

Hi, I added the deadline to the Scrive CAT - it aims to be 26.1.2025, but if you make it sooner, than let us know sooner.

Root user group can't use inheritance. - what does it mean? should it be empty fields if there is "inherited_from": "1" and "parent_id": null ? or it just should be editable fields in that case?

It means that it should not be possible to get into this state. Pick whatever solution you see fit for this case from your experience and best judgement.

scrive-cat-bot · 2025-01-16T21:09:56Z

🥷 🥷 It's ready for review, I suppose. :)

kraklin · 2025-01-17T09:53:53Z

🥷 🥷 It's ready for review, I suppose. :)

Great. Feel free to update the Description of the PR with the description of your thinking process or challenges that you had during implementation if you feel like there is some context for us to know or take into account during reviewing.

kutyel · 2025-01-17T10:03:29Z

src/Modules/Tags.elm

+                        |> Dict.toList
+                        |> List.map (Tuple.second >> .name)
+                        |> List.filter ((==) model.newTagName)
+                        |> List.head
+                        |> Maybe.map (\_ -> True)
+                        |> Maybe.withDefault False


The code is correct, but there are simpler ways of doing this like, for example, Dict.foldl or maybe one of the Dict.Extra functions if you want to use additional packages (we often do 😉 )

Dict.Extra.any =|

Dict.values >> List.any ((==) model.newTagName) if you don't want to use Dict.Extra.

kutyel · 2025-01-17T10:05:02Z

src/Shared/Styles.elm

@@ -0,0 +1,16 @@
+module Shared.Styles exposing (..)


mmhh, styles module on a different file... interesting... 🤔 I think one of the points of doing tailwind is precisely to allocate the styles as close to the actual code as possible... IMHO

Different approach than we are used to 🤷 Not necessarily bad part

I actually think it's good they're trying to keep the style consistent.

🥷 🥷 To be honest, at the end, I came up with just splitting style classes into several attributes, so I had readable tailwind styles at place without need to go to separate module to check, what were the styles there.

🥷 🥷 So I would agree that extracting tailwind styles into separate module isn't good idea.

kutyel · 2025-01-17T10:12:00Z

elm.json

@@ -6,10 +6,12 @@
    "elm-version": "0.19.1",
    "dependencies": {
        "direct": {
+            "NoRedInk/elm-json-decode-pipeline": "1.0.1",


I like the usage of this library, I feel we should use it more often 💯

It's a bit dangerous, because it relies on the order that attributes are declared in a record.
Change the order, you get a very nasty and very silent bug.
Bit us a couple of times at a previous job of mine.

xarvh · 2025-01-17T10:36:52Z

src/Modules/Tags.elm

+              else if String.length model.newTagName > 32 then
+                { model | newTagError = Just "too long (32)" }
+
+              else


It feels like we're implementing the tag validation twice (second time is https://github.com/scrive/elm-challenge/pull/16/files#diff-edc7374f17c064fedb4f48d570e9455210c16d868744151dd8cf1d725c3f011fR180 )

This is a maintenance problem.

🥷 🥷 Oh, you're right. Sorry, haven't noticed it. Is it legal if I fix it now?

Sure it is legal :) This task is thought as being as close to a regular day to day job in Scrive as possible and as such part of the discussion about the code in PR are some changes on stuff that we find out can be improved.

Although just do the changes in bulk and ideally once as we need to cover more candidates and it might become little bit messy and we might lost track of what has been changed and when.

Eh, if it's just a mistake there's no need to fix it. Happens. =)

xarvh · 2025-01-17T10:39:45Z

src/Modules/Tags.elm

+                { model | newTagError = Just "empty" }
+
+              else if String.length model.newTagName > 32 then
+                { model | newTagError = Just "too long (32)" }


Adding errors to the model, rather than running the validation function with every view call, requires us to remember to clear the errors every time.
It's a solution that increases maintenance overhead and exposes us to bugs.

🥷 🥷 I think that I didn't get your point here. I we add validation to view, so it would be called every time model changes and show us an error immediately, it could be a bit distracting for user to get errors while editing data (like when you want to add tag with name "Liked by others" it would trigger an error for a moment if there is tag with name "Like"). So I decided to validate data only on submit, when user ended up editing stuff and thinks he's ready to finish.

I would have something like a "showValidation" flag in the Model, that we set on input blur or (if we really want to be fancy) on a debounced delay after the user has started typing.
Still, good thinking!

src/Main.elm

xarvh · 2025-01-17T10:50:13Z

src/Main.elm

+emptyUserGroup : UserGroup
+emptyUserGroup =
+    { id = ""
+    , parentId = ""


In the model, newTagError is a Maybe String, ie, the "no error" condition is modeled by Nothing.
Here instead, the "no parent" condition is modeled by "".

There isn't really a correct way to do this, but I would like to hear from the candidate about why they choose different approaches in the two instances.

🥷 🥷 I've just spllitted challenge into 3 days, 1 day for each task, and decoders were written in the 1st day while tags task was completed at 3rd, so I just haven't notices that I have different approaches there, because decoders were working fine and I had no reason to look there and change them. :)

raadecki · 2025-01-17T11:04:34Z

src/Modules/Tags.elm

+            |> Input.withOnChange (Just NewTagNameChanged)
+            |> Input.withErrorMessage model.newTagError
+            |> Input.withValue model.newTagName
+            |> Input.viewTextOrNumber


do you have an idea how to improve readability of this part (maybe how to replace builder pattern)?

🥷 🥷 I think so, maybe it would be better if it could take just List of arguments. Like this Input.viewTextOrNumber [Input.label "e-mail" , Input.error (Just error)]

xarvh · 2025-01-17T12:34:42Z

src/Modules/Settings.elm

+            (List.singleton
+                >> List.filter ((/=) 0)
+                >> List.head
+            )


This seems a bit over engineered.
I'd go with a simple

case maybeInt of Just 0 -> Nothing _ -> maybeInt

adjust contact validation logic

scrive-cat-bot · 2025-01-17T13:38:35Z

🥷 🥷 Just fixed some comments and bugs I found during code-re-review :)

Anonymous added 12 commits January 15, 2025 09:41

COMMIT-1 Implement data structure and decoders

c6013f8

add font

9dfedd6

add default empty data implement preferred contact method input

implement an email field

4cffab6

implement phone input

1a14a65

implement reusable input

implement company name input

b9c2c52

implement address fields

12bc714

implement buttons

ceb705b

add email validation and error message

87d0c60

add phone validation

2b7cbbd

adjust styling

b481d96

validate address

4222e9f

make form more responsive

3447b18

extract form

9d4204e

Anonymous added 2 commits January 15, 2025 16:13

add data retention settings section

3da57e2

implement data retention policy form

fcd67a2

Anonymous added 11 commits January 16, 2025 09:56

extract contact form into separate module

4596085

adjust contact form init

37708c4

extract setting to module

f74da3e

focus on policy field after add

d1b874d

add tags view

92c29d1

add tags module & extract data structures into modules

7400295

implement tags form view

ea1ebd2

add 'remove' button

0fa0d4a

add functionality to tags form

460c0fa

adjust input logic

03c4eb5

add tags validation

367dadb

Anonymous added 6 commits January 16, 2025 19:58

reset tags form on cancel/close

6e02a14

fix forms states

660610f

add value field to new tag, let it be submitted by enter

677c7c6

add remove button to settings

dcceca1

check whole contact form for validity on sumbit

40b8115

fix inheritance logic

98590c2

expose only required stuff

8b9a12b

kraklin added the ready for review Marks candidate task to be ready for review label Jan 17, 2025

kraklin requested review from xarvh and raadecki January 17, 2025 09:54

kutyel reviewed Jan 17, 2025

View reviewed changes

xarvh reviewed Jan 17, 2025

View reviewed changes

src/Main.elm Show resolved Hide resolved

xarvh reviewed Jan 17, 2025

View reviewed changes

raadecki reviewed Jan 17, 2025

View reviewed changes

Anonymous added 3 commits January 17, 2025 14:22

adjust tags validation logic/ remove duplication

26290d6

remove unnecessary styles module

39022b4

extract big views into functions

21f7e31

xarvh reviewed Jan 17, 2025

View reviewed changes

Anonymous added 4 commits January 17, 2025 15:04

use string approach for errors

1fe9912

adjsut inheritance logic

c0f3c36

adjust contact validation logic

use simple approach for nonZero

cf6c48b

improve input build readability

f307431

kraklin closed this Feb 21, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Task solution from happy-luna #16

Task solution from happy-luna #16

kraklin commented Jan 15, 2025 •

edited by scrive-cat-bot

Loading

kraklin commented Jan 15, 2025

kraklin commented Jan 15, 2025

kraklin commented Jan 15, 2025

kraklin commented Jan 15, 2025

scrive-cat-bot commented Jan 16, 2025

kraklin commented Jan 17, 2025

kutyel Jan 17, 2025 •

edited

Loading

xarvh Jan 17, 2025

xarvh Jan 17, 2025 •

edited

Loading

kutyel Jan 17, 2025 •

edited

Loading

kraklin Jan 17, 2025

xarvh Jan 17, 2025

scrive-cat-bot Jan 17, 2025

scrive-cat-bot Jan 17, 2025

kutyel Jan 17, 2025

xarvh Jan 17, 2025

xarvh Jan 17, 2025

scrive-cat-bot Jan 17, 2025

kraklin Jan 17, 2025

xarvh Jan 17, 2025

xarvh Jan 17, 2025

scrive-cat-bot Jan 17, 2025

xarvh Jan 17, 2025

xarvh Jan 17, 2025

scrive-cat-bot Jan 17, 2025

raadecki Jan 17, 2025

scrive-cat-bot Jan 17, 2025

xarvh Jan 17, 2025

scrive-cat-bot commented Jan 17, 2025

Task solution from happy-luna #16

Task solution from happy-luna #16

Conversation

kraklin commented Jan 15, 2025 • edited by scrive-cat-bot Loading

kraklin commented Jan 15, 2025

kraklin commented Jan 15, 2025

kraklin commented Jan 15, 2025

kraklin commented Jan 15, 2025

scrive-cat-bot commented Jan 16, 2025

kraklin commented Jan 17, 2025

kutyel Jan 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xarvh Jan 17, 2025 • edited Loading

Choose a reason for hiding this comment

kutyel Jan 17, 2025 • 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

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

Choose a reason for hiding this comment

scrive-cat-bot commented Jan 17, 2025

kraklin commented Jan 15, 2025 •

edited by scrive-cat-bot

Loading

kutyel Jan 17, 2025 •

edited

Loading

xarvh Jan 17, 2025 •

edited

Loading

kutyel Jan 17, 2025 •

edited

Loading