Task solution from sleepy-luna #41

scrive-cat-bot · 2025-12-01T11:48:51Z

This PR contains solution from the candidate sleepy-luna

xarvh · 2025-12-02T09:10:38Z

src/AddTagForm.elm

+    Html.span [ Attributes.class "text-red-500", Attributes.attribute "aria-hidden" "true" ] [ Html.text "\u{200A}*\u{00A0}" ]
+
+
+type alias FormModel msg =


Why call it FormModel instead than just Model?

In fact, why call it Model at all, since it seems to be used just as a convenient way to express view's parameters?

Good point, maybe ViewModel could be a better name, but since there are multiple parameters with same type, one could easily mismatch the order mixing the parameters. In that case, creating a record like this where each parameter is named makes it clear and makes it less likely somebody would mix addTagMsg and cancelMsg

xarvh · 2025-12-02T09:23:21Z

src/Types.elm

+
+type alias Tag =
+    { name : String
+    , value : Maybe String


What is the difference between value being Nothing and it being Just ""?

when value is Nothing it means it does not exist, it was not provided or was not setup at all. While having value Just "" the tag exists with value of empty string. Somebody explicitly provided the value, user chose to put empty string as value for example by submitting a form with empty field.

kraklin · 2025-12-02T09:27:55Z

TODOs.md

+
+## Questions & Comments
+
+- I was not sure how much I should extend the setup or the project packages, but I've found out that when the project was built the tailwind didn't work in build so I've done some changes to make it work.


You can extend the setup to your liking :)

I see, I was not sure if this is something that is there for me to spot and point out in the task, i've added it here to not forget to ask about it. Since there are some breaking changes and the setup is a bit different for tailwind 4 I think I will keep it as is for now :D

xarvh · 2025-12-02T09:33:03Z

src/Main.elm

+    , newTag : Maybe Tag
+    , editingTagIndex : Maybe Int
+    , editingTagValue : String
+    , addTagError : Maybe String


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 "A tag with this name already exists" error does not disappear

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

xarvh · 2025-12-02T09:38:18Z

The button to confirm deletion appears exactly under the delete icon, a misclick may result in deletion.

Edit icon is not keyboard friendly, its color does not allow to see the focus outline.

Update function feels a bit complex, difficult to follow. There is a lot of status juggling which might result in errors.

This said, the code is generally clean and well structured.

xarvh · 2025-12-02T09:39:09Z

src/AddTagForm.elm

+
+requiredAsterisk : Html msg
+requiredAsterisk =
+    Html.span [ Attributes.class "text-red-500", Attributes.attribute "aria-hidden" "true" ] [ Html.text "\u{200A}*\u{00A0}" ]


There is no explanation anywhere of what the asterisk means.

I forgot to add explanation for the asterisk. I'll fix that, thanks.

kraklin

Thanks for submitting your engineering task. I have left few questions here and there.

I quite like your solution, especially that extra ARIA considerations like aria-labelledby and that aria-live panel for screen readers only.

I also like the timeout you put on the deletion button, making it clear that you really want to delete the tag. Although it might be quite easy to double click by accident, so I'd probably switch the timeout around - put some delay on the button after the first click, so you don't misclick it twice.

The splitting to smaller modules is nice touch as well and makes the code more readable and reviewable.

On things that I don't like in general are nested ifs and mixing presentation code with logic code - having ifs and case expression inside the view makes it IMHO harder to reason about the final look as you need to keep all the branching conditions in your head.

We can discuss questions and your points in TODO.md on our call later.

kraklin · 2025-12-02T09:30:25Z

src/AddTagForm.elm

+        , case formErrorMessage of
+            Just error ->
+                Html.div [ Attributes.class "p-3 bg-rose-100 border border-rose-400 text-rose-700 rounded" ]
+                    [ Html.text error ]
+
+            Nothing ->
+                Html.text ""


There is Html.Extra.viewMaybe if you'd like to get rid of the case expression here: https://package.elm-lang.org/packages/elm-community/html-extra/3.4.0/Html.Extra#viewMaybe

👍, so many great extra packages and functions, yes that would make it a bit more clean

kraklin · 2025-12-02T09:31:57Z

src/AddTagForm.elm

+                    , Attributes.required True
+                    , Attributes.placeholder "Enter name, e.g. 'Powered by'"
+                    , Attributes.maxlength 32
+                    , Attributes.attribute "aria-describedby" "tag-name-max-length"


aria-describedby.. nice 👍

kraklin · 2025-12-02T09:33:02Z

src/AddTagForm.elm

+                    , Attributes.id "tag-name-input"
+                    , Attributes.required True
+                    , Attributes.placeholder "Enter name, e.g. 'Powered by'"
+                    , Attributes.maxlength 32


Why have you choose the limit to be 32?

In the task-tags.md under requirements, one of the requiremets is "Names should not be longer than 32 characters". So I've added it here as a maxlength attribute.

On the second though I should probably also validate this inside update, this only prevents user from entering more than 32 characters manually but you could pass in the value directly through JS which the maxlength will not prevent.

kraklin · 2025-12-02T09:35:11Z

src/Button.elm

+        , label : String
+        , leftIcon : Maybe (Html msg)
+        , rightIcon : Maybe (Html msg)
+        , onClickMsg : Maybe msg


Why is onClickMsg a Maybe? When would the Nothing case be useful?

I made it Maybe so that I don't have to create a new button with a msg or have a button with message "NoOp" :D

In my use case when I'm submitting the form, the fact that the button is a submit type inside a form will trigger onSubmit msg of the form, or if the button is a submit button connected to a form via form attribute, so the button itself don't need a msg.
I guess also if you wanted to have a decorative button, or button that sometimes does something and sometimes does not depending on the data it gives you option to pass in Nothing but for that maybe I should have disabled : Bool as an option.
One more thing I can think of is if I have a Button that is a link that looks like a button, i could have a button variant that does not need a msg. That probably can also be solved by more complex button API instead.

kraklin · 2025-12-02T09:38:03Z

src/Button.elm

+withIconLeft : Maybe (Html msg) -> Button msg -> Button msg
+withIconLeft icon (Settings model) =
+    Settings { model | leftIcon = icon }
+
+
+withIconRight : Maybe (Html msg) -> Button msg -> Button msg
+withIconRight icon (Settings model) =
+    Settings { model | rightIcon = icon }


IMHO withIconRight and withIconLeft doesn't need to have Maybe (Html msg) as the default would be Nothing case and when I'm setting the icon here, I can't find a case where I'd pass Nothing here instead.

Can you provide a scenario why that Maybe (Html msg) would be useful? Also what would Maybe (Html Never) do to this logic?

There might be some scenarios where the Maybe will work well for instance let's say we show a spinner inside a button while data is being sent to backend or something is loading, if we would have something like:

Button.new { class = "xyz", label = "Submit form" } |> Button.withIconRight (if isLoading then Just Icon.Spinner else Nothing) |> Button.view

this will be more clean than using

if isLoading then Button.new { class = "xyz", label = "Submit form" } |> Button.withIconRight Icon.Spinner |> Button.view else Button.new { class = "xyz", label = "Submit form" } |> Button.view

Additionally using Html Never would be much better for the icons so that the icon cannot produce any msg. In case somebody would create a bad icon that has a onClick event on it, you would not be able to use it with the Button as it will raise a type error. Thanks for that suggestion.

So I think the most flexible and typesafe it would be Maybe (Html Never) but I guess I would consider that over Html Never -> Button msg -> Button msg in withIconRight withIconLeft based on the usage in the app and if we ever need something like conditional icons I mentioned above.

kraklin · 2025-12-02T09:39:27Z

src/Button.elm

+            ++ (case model.onClickMsg of
+                    Just msg ->
+                        [ Events.onClick msg ]
+
+                    Nothing ->
+                        []
+               )


There is elm-community/html-extra function for this :)

https://package.elm-lang.org/packages/elm-community/html-extra/3.4.0/Html.Attributes.Extra#attributeMaybe

kraklin · 2025-12-02T09:51:42Z

src/Main.elm

+            case List.Extra.getAt index model.tags of
+                Just tag ->
+                    ( { model | editingTagIndex = Just index, editingTagValue = Maybe.withDefault "" tag.value, ariaLiveMessage = Just ("Editing tag " ++ tag.name) }
+                    , Task.attempt (\_ -> NoOp) (Dom.focus "edit-tag-value-input")


Nice touch with autofocusing the input field 👍

kraklin · 2025-12-02T09:53:35Z

src/Main.elm

+                    if pendingIndex == index then
+                        if Just index == model.editingTagIndex then
+                            ( { model
+                                | tags = List.Extra.removeAt index model.tags
+                                , editingTagIndex = Nothing
+                                , editingTagValue = ""
+                                , ariaLiveMessage = Just "Tag deleted successfully"
+                                , pendingDeleteOnIndex = Nothing
+                              }
+                            , Cmd.none
+                            )
+
+                        else
+                            ( { model
+                                | tags = List.Extra.removeAt index model.tags
+                                , ariaLiveMessage = Just "Tag deleted successfully"
+                                , pendingDeleteOnIndex = Nothing
+                              }
+                            , Cmd.none
+                            )
+
+                    else
+                        ( { model
+                            | pendingDeleteOnIndex = Just index
+                            , ariaLiveMessage = Just "Click delete again to confirm"
+                          }
+                        , Task.perform (\_ -> CancelPendingDelete index)
+                            (Task.succeed ()
+                                |> Task.andThen (always (Process.sleep 3000))
+                            )
+                        )


These nested ifs are liiitle bit hard to read. Any idea how you'd make this part of the code better?

I guess first thing that comes to mind is to change it so that there is a DeleteTag index msg and a new message that is something like ConfirmDeleteTag index which is triggered from the button "Confirm delete" instead of the current DeleteTag index as now I have the same msg that handles both flows. I'll change that and lets see how much it helps.

scrive-cat-bot · 2025-12-03T22:59:10Z

Yeah I'm not really happy with the buttons, I've added the confirm flow to make it less likely that somebody deletes a tag by accident but I guess it's still not ideal.

I believe the color of the outline depends on platform, browser and theme. For instance on MacOS I have the default outline blue which is visible ok on black buttons, What if I add outline-offset on *:focus-visible to make it bit more clear even if the device adds black outline? Would that be ok? Alternatively we could override the color and styles of the outline altogether which could cause other issues.

I'll see what I can do with the update to make it more clear.

- Add Html.Extra package and use it to simplify some views - Simplify update by introducing ConfirmDeleteTag msg - add global outline-offset to make the focusing more clear - switch icons to Html Never - explain *

kraklin and others added 2 commits November 25, 2025 15:55

Just testing stuff, nothing to worry about

d257b22

Implement tags management form

53bb16d

scrive-cat-bot added the ready for review Marks candidate task to be ready for review label Dec 1, 2025

xarvh reviewed Dec 2, 2025

View reviewed changes

kraklin reviewed Dec 2, 2025

View reviewed changes

xarvh reviewed Dec 2, 2025

View reviewed changes

kraklin reviewed Dec 2, 2025

View reviewed changes

Implement changes from PR feedback

9c0ac6a

- Add Html.Extra package and use it to simplify some views - Simplify update by introducing ConfirmDeleteTag msg - add global outline-offset to make the focusing more clear - switch icons to Html Never - explain *

		Html.span [ Attributes.class "text-red-500", Attributes.attribute "aria-hidden" "true" ] [ Html.text "\u{200A}*\u{00A0}" ]


		type alias FormModel msg =


		## Questions & Comments

		- I was not sure how much I should extend the setup or the project packages, but I've found out that when the project was built the tailwind didn't work in build so I've done some changes to make it work.

Task solution from sleepy-luna #41

Are you sure you want to change the base?

Task solution from sleepy-luna #41

Uh oh!

Conversation

scrive-cat-bot commented Dec 1, 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!

xarvh commented Dec 2, 2025

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!

scrive-cat-bot commented Dec 3, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants