Task solution from fierce-rabbit #40

scrive-cat-bot · 2025-11-28T14:33:53Z

This PR contains solution from the candidate fierce-rabbit

Bumps [postcss](https://github.com/postcss/postcss) from 8.4.29 to 8.4.31. - [Release notes](https://github.com/postcss/postcss/releases) - [Changelog](https://github.com/postcss/postcss/blob/main/CHANGELOG.md) - [Commits](postcss/postcss@8.4.29...8.4.31) --- updated-dependencies: - dependency-name: postcss dependency-type: direct:development ... Signed-off-by: dependabot[bot] <support@github.com>

Bump postcss from 8.4.29 to 8.4.31

kraklin · 2025-11-28T14:58:00Z

Is this ready for review or are there going to be more changes?

kraklin · 2025-11-28T14:59:06Z

README.md

 npm ci
 npm start
 ```
+test stuff


This is here from testing out the scrive-cat deployment and accidentally commit to master. Please ignore this line.

scrive-cat-bot · 2025-11-29T17:08:34Z

Ready for review I would say. Of course, there is still space for improvement but it's always. :)

xarvh · 2025-12-01T10:05:58Z

src/Main.elm

+                        Value.addError "Name is required" nameValue
+
+                    else if List.any (.name >> Editable.valueFromEditable >> Value.fromValue >> (==) nameString) model.tags then
+                        Value.addError "Duplicate name" nameValue


Storing your validation error messages in the Model is a bad idea, they are redundant information.

Case in point:

Enter a duplicate Tag Name (an error hint will correctly display)

Delete the duplicate tag

The "duplicate" error hint does not disappear

This approach makes your update function more complicated than it needs to be and more prone to errors.

Thanks for the suggestion! Just to clarify, in this code globalError only stores JSON decoding errors.
Validation errors for inputs (like duplicate tag names) are already handled directly in their Value, so they dynamically update without storing redundant information in the Model.

Thanks for the suggestion! Just to clarify, in this code globalError only stores JSON decoding errors.
Validation errors for inputs (like duplicate tag names) are already handled directly in their Value, so they dynamically update without storing redundant information in the Model.
But for the scenario that you mentioned, I added validation also for ClickedRemoveTag message.

Both globalError and the error stored inside a Value are redundant, because they can be derived from information you have already.

In the case of globalError, its value can always be derived from the JSON.
We could say that we are pretending we got that data from a remote server, but in that case I think it would be better to store the Result produced by Json.decodeString.

In the case of Value, we can derive the error simply from the Model.
We just need the input value and the list of the other tags to know to know whether it is duplicate or not.
Storing the error message inside Value is redundant and results in a (minor, in this case) bug.

xarvh · 2025-12-01T10:09:03Z

src/Main.elm

+emptyNewTagForm : Tag
+emptyNewTagForm =
+    { name = Value.toValue "" |> Editable.valueToEditable
+    , value = Value.toValue "" |> Editable.valueToEditable


This reads a bit weird. Value.toValue sounds like I take a Value and transform it into a Value.
Perhaps Value.fromString?

The function works for any type of value, not just String. I could rename it to something like Value.validValue to make that clearer.

Wouldn't Value.valid be enough?

This function works for any type of value, not just String. I could rename it to something like Value.validValue to make that clearer

xarvh · 2025-12-01T10:19:46Z

src/Main.elm

+                updatedTags =
+                    List.map
+                        (\tag ->
+                            if (Editable.valueFromEditable tag.name |> Value.fromValue) == nameString then


I think Editable.toValue would read better.

xarvh · 2025-12-01T10:22:58Z

src/Main.elm

---- VIEW ----
+        ClickedRemoveTag nameToRemove ->
+            ( { model
+                | tags = List.filter (.name >> Editable.valueFromEditable >> Value.fromValue >> (/=) nameToRemove) model.tags


I like the systematic approach, but OTOH we need to keep unwrapping (twice!) and wrapping these Strings and I'm not sure there's a clear benefit, it feels like it may be perhaps a bit overengineered?

I understand your constrain and you're right - in such a simple project it's not necessary.
I mainly used this approach to demonstrate that I’m comfortable working with more advanced data structures and patterns, even if they aren’t strictly required here.

Fair enough. =)

xarvh · 2025-12-01T10:26:01Z

src/Main.elm

 subheader : String -> Html msg
 subheader text =
-    Html.span [ Attrs.class "p-2 text-2xl font-extrabold text-slate-800" ]
+    Html.span [ Attrs.class "p-2 text-2xl font-extrabold text-[#1A1A1A]" ]


Why did you decide to use color literals instead than constants?

kraklin

Thanks for the solution to our hiring task. I quite like it. Splitting the UI parts away to separate modules in Shared is a nice touch for sure.

UI is clear and navigable by keyboard with usage of aria tags and/or titles for image icons - nice :) I'd add the possibility to enter the tag from the form by hitting Enter as I was doing that repeatedly when trying to navigate only by keyboard.

Code reads mostly good, there is one or two places that feels too unnecessarily nested to me.

I left some suggestions and questions in the comments. Can you expand on them there? 🙏

kraklin · 2025-12-01T09:38:49Z

src/Shared/Tooltip.elm

+view : String -> String -> Html msg
+view id tooltipText =


Why did you choose the view to have String -> String type? Do you know why and under what circumstances it is a problem and how to mitigate it?

I agree, String types can be accidentally swapped, so it's better to use record in cases like this. I will fix it.

kraklin · 2025-12-01T09:40:18Z

src/Shared/Icon.elm

+type IconType
+    = Add
+    | Edit
+    | Checkmark
+    | Refresh
+    | Remove


Why did you go with type variant and not exposing the icons as separate functions? What are the benefits of this approach? What are the drawbacks?

I chose to use a type variant IconType because it's simple and consistent. It allows calling Icon.view Icon.Add uniformly and easily mapping over lists of icons.
The main drawback is that every render requires a case expression, and it’s slightly less flexible if we need customization.
Using separate functions for each icon would avoid the case but make it harder to dynamically work with collections of icons.

In my solution I used inspiration from elm components pattern (https://elm.land/concepts/components.html) what is approach that makes sense to me :)

kraklin · 2025-12-01T09:46:08Z

src/Shared/Value.elm

+toValue : a -> Value a
+toValue =
+    Valid


You expect the value to be valid straight away during the conversion to Value. What if we'd like to validate this value based on some validators? Would you do some changes to the Value module in such case or would you keep it as it is?

I chose to have toValue create a Valid value directly because my goal was mainly to demonstrate the Value module.
I didn’t want to go into unnecessary details of full validation logic for this task, but the module could be extended with validators if needed.

kraklin · 2025-12-01T09:47:41Z

src/Shared/Value.elm

+isValid : Value a -> Bool
+isValid value =
+    case value of
+        Invalid _ _ ->
+            False
+
+        Valid _ ->
+            True
+
+
+isInvalid : Value a -> Bool
+isInvalid value =
+    case value of
+        Invalid _ _ ->
+            True
+
+        Valid _ ->
+            False


IMHO this could be simplified:

isInvalid = not << isValid

Good catch, I overlooked this one - thanks for pointing it out :)

kraklin · 2025-12-01T09:51:14Z

src/Shared/Editable.elm

+updateIfEditable : Editable a -> a -> Editable a
+updateIfEditable editable newValue =


I'd IMHO switch the order of parameters for better composability to a -> Editable a -> Editable a. Why did you use this order here?

You're right, switching the parameters would make it easier to use this function with mapping, and it also aligns better with Elm conventions.

kraklin · 2025-12-01T09:52:54Z

src/Shared/Button.elm

+        ( tooltipId, tooltipAttributes ) =
+            let
+                tooltipId_ =
+                    "tooltip-" ++ Maybe.withDefault "button" label


Under which circumstances this can backfire? And how would you mitigate the problem?

Which exact part of the code do you mean? I’ve committed a few times in the meantime, so I’m not sure.

Sorry, I mean the fact that you are creating element ID from label. What might be the problem here?

kraklin · 2025-12-01T10:14:23Z

src/Main.elm

+                        Value.addError "Name is required" nameValue
+
+                    else if List.any (.name >> Editable.valueFromEditable >> Value.fromValue >> (==) nameString) model.tags then
+                        Value.addError "Duplicate name" nameValue


The nesting of let .. in inside let .. in is a codesmell to me at it might be quite challenging to follow which value is from where and it adds complexity to overall code readability. Would it be possible to refactor it out? How?

overall lots is going on inside upadte function. Do you have an idea how to make it more readable?

I understand that this can make the code harder to read. One solution could be to use a single let...in block, or better, to extract parts of the logic into separate helper functions. In a real project, I would likely create a dedicated module for validators to keep the code clean and modular.

kraklin · 2025-12-01T10:17:47Z

src/Main.elm


 type alias Model =
-    {}
+    { tags : List Tag


Why you choose to use List here instead of e.g. Dict? Can you elaborate?

I chose List because the number of tags is expected to be small and operations like adding, removing, or iterating over them are straightforward.
Using a Dict could be beneficial if we needed fast lookups by key or expected a very large collection, but for this task a List keeps the code simpler and more readable.

kraklin · 2025-12-01T10:21:12Z

src/Main.elm

+                     ]
+                        ++ (if Editable.isEditable editable then
+                                []
+
+                            else
+                                [ Attrs.attribute "readonly" "true" ]
+                           )
+                    )


There is Html.Attributes.Extra.attributeIf in Html.Extra package that is quite helpful with these conditional attributes.

I didn't know this but I love it, thanks.

Very useful and elegant, thanks!

I love it, thank you :)

I love it, thanks :)

I love this, thanks for sharing :)

I love this, thank you :)

kraklin · 2025-12-01T10:22:03Z

src/Main.elm

+                , if Editable.isEditable editable then
+                    Html.text ""
+
+                  else
+                    Html.div
+                        [ Attrs.class "pointer-events-none absolute inset-0 flex items-center px-3 text-gray-700"
+                        ]
+                        [ Html.text valueString ]


There is Html.Extra.viewIf in the Html.Extra package that shines in situations like these.

Very useful and elegant, thanks!

xarvh · 2025-12-01T10:26:55Z

src/Main.elm

+    let
+        newTagFormView =
+            Html.div [ Attrs.class "mb-6 grid grid-cols-1 md:grid-cols-[2fr_2fr_auto] gap-3 items-center w-full max-w-full" ]
+                [ inputField "Tag Name *" model.newTagForm.name UpdateNewName


There is no explanation of what "*" means.
I would but a more explicit "(required)".

xarvh · 2025-12-01T10:28:57Z

src/Main.elm

+                ]
+            >> List.singleton
+        )
+        >> Maybe.withDefault []


I feel like a case..of here would be more readable.

xarvh · 2025-12-01T10:32:11Z

src/Main.elm

+
+                            else
+                                [ Attrs.attribute "readonly" "true" ]
+                           )


I'm not 100% sure of the accessibility implications of creating an <input> field just to set it to readonly.

xarvh · 2025-12-01T10:38:13Z

src/Shared/Button.elm

+         , Attrs.attribute "aria-label" (Maybe.withDefault "Button" label)
+         ]
+            ++ tooltipAttributes
+            ++ [ Events.onClick onClickMsg ]


Why is this added by itself rather than together with the first List of attributes?

Sorry, I’ve already pushed a few changes, so I’m not sure which part of the code this comment is about.

xarvh · 2025-12-01T10:40:44Z

src/Shared/Icon.elm

+                    , SvgAttrs.d "M6 18L18 6M6 6l12 12"
+                    ]
+                    []
+                ]


What is the advantage of having all the icons in a single function rather than each their own constant?
(ie, Shared.Icon.add, Shared.Icon.edit... and so on)

(Nevermind, I see Tomas asked the same question.)

xarvh · 2025-12-01T10:46:27Z

The Shared modules seem to be designed to have their own code clean rather than to allow the user code to be clean.
ie: instead of hiding complexity inside themselves, they offload it to the user code.

The UI feels good, is clear and easy to interact with.
The layout is very nice.

There is a bit of overengineering, but I'll write it off as the candidate having fun with Elm, which is a plus. =)

scrive-cat-bot · 2025-12-03T18:38:31Z

Thanks! I see what you mean about the Shared modules. I agree it adds some complexity to the user code, but I wanted to show working with reusable components. I’m glad that you like the visual part!
And thank you all for your feedback, I really appreciate it :)

xarvh · 2025-12-04T08:59:35Z

No worries.
Also, sorry for the barrage of comments.
My intention was to poke you and see how you think, but rereading our whole interaction and given that you are a candidate I fear my writing came off as overly judgmental.

scrive-cat-bot · 2025-12-04T09:56:49Z

No worries at all — I didn’t take it as overly critical.
One thing I forgot to mention: for Shared.Icon I used the components pattern (https://elm.land/concepts/components.html
), because that approach makes sense to me.

Anonymous added 7 commits October 8, 2023 02:58

Merge pull request #1 from scrive/dependabot/npm_and_yarn/postcss-8.4.31

ce2d17a

Bump postcss from 8.4.29 to 8.4.31

Update README.md

3e1ba0b

Update README.md for one task only

092f11d

Update README.md for one task only

566723a

Just testing stuff, nothing to worry about

2907f89

Taga management form

520f94b

kraklin reviewed Nov 28, 2025

View reviewed changes

kraklin added the ready for review Marks candidate task to be ready for review label Dec 1, 2025

xarvh reviewed Dec 1, 2025

View reviewed changes

kraklin reviewed Dec 1, 2025

View reviewed changes

xarvh reviewed Dec 1, 2025

View reviewed changes

src/Main.elm

]

>> List.singleton

)

>> Maybe.withDefault []

Copy link

xarvh Dec 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like a case..of here would be more readable.

xarvh reviewed Dec 1, 2025

View reviewed changes

Anonymous added 7 commits December 1, 2025 13:01

Add tag onSubmit

fbc959b

Use record for Tooltip arguent

c4ecd33

Simplify Value.isInvalid function

140cbe6

Change order of updateIfEditable function's arguments

e691496

Extract validation function

04bd3a5

Use AttrsExtra.attributeIf

af9fa7c

Use Html.Extra.viewIf

63e5f95

Anonymous added 2 commits December 1, 2025 14:14

Validate tag name also when removing tag

59c7e2e

Rename toValue to validValue

a74c094

scrive deleted a comment from scrive-cat-bot Dec 1, 2025

Remove redundant attribute from input

53f09b0

scrive deleted a comment from scrive-cat-bot Dec 1, 2025

Anonymous added 2 commits December 3, 2025 19:11

Rename Editable.valueFromEditable to Editable.toValue

8090ff6

Change * sign to "required"

006a1e7

scrive-cat-bot force-pushed the anonymized-fierce-rabbit branch from 3c8379a to 006a1e7 Compare December 5, 2025 12:18

		updateIfEditable : Editable a -> a -> Editable a
		updateIfEditable editable newValue =

Task solution from fierce-rabbit #40

Are you sure you want to change the base?

Task solution from fierce-rabbit #40

Uh oh!

Conversation

scrive-cat-bot commented Nov 28, 2025

Uh oh!

kraklin commented Nov 28, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scrive-cat-bot commented Nov 29, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kraklin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!