-
Notifications
You must be signed in to change notification settings - Fork 2
Task solution from sleepy-luna #41
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,2 +1,5 @@ | ||
| node_modules | ||
| elm-stuff | ||
|
|
||
| dist | ||
| .DS_Store |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| 22.21.0 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -44,3 +44,4 @@ Good luck and enjoy the challenge. | |
| npm ci | ||
| npm start | ||
| ``` | ||
| test stuff | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| # TODOs, questions and suggestions | ||
|
|
||
| ## TODOs | ||
|
|
||
| - [x] Make the mobile less crowded | ||
| - [x] Make sure save button is connected to the form | ||
| - [x] BUG: Removing currently edited tag messes up the next one | ||
| - [x] Confirm deletion | ||
| - [ ] Mobile layout is a bit silly | ||
| - [-] Toast or aria-live to describe actions - partially working | ||
| - [ ] Extract accessibility attributes into a module | ||
| - [ ] | ||
|
|
||
| ## 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. | ||
| - New version of tailwind is out for quite some time, do we want to upgrade? | ||
|
|
||
| ## Suggestions | ||
|
|
||
| - add `.nvmrc`, `.node-version` files or `engines` field in packajge.json to make sure people use the same node version | ||
| - consider moving away from `npm` to `pnpm` or other modern alternative | ||
| - consider using `corepack` and `packageManager` field in package.json | ||
| - review outdated dependencies and audit | ||
| - `elm-review` could be a nice addition to the project | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,13 +1,10 @@ | ||
| <!DOCTYPE html> | ||
| <html lang="en"> | ||
| <head> | ||
| <meta charset="UTF-8"> | ||
| <meta name="viewport" content="width=device-width, initial-scale=1.0"> | ||
| <link media="all" rel="stylesheet" href="src/styles.css"> | ||
| <title>Elm challenge</title> | ||
| <script type="module" src="src/app.js"></script> | ||
| </head> | ||
| <body> | ||
|
|
||
| </body> | ||
| <head> | ||
| <meta charset="UTF-8" /> | ||
| <meta name="viewport" content="width=device-width, initial-scale=1.0" /> | ||
| <title>Elm challenge</title> | ||
| <script type="module" src="src/app.js"></script> | ||
| </head> | ||
| <body></body> | ||
| </html> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,6 @@ | ||
| module.exports = { | ||
| export default { | ||
| plugins: { | ||
| tailwindcss: {}, | ||
| autoprefixer: {}, | ||
| }, | ||
| } | ||
| }; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,98 @@ | ||
| module AddTagForm exposing (view) | ||
|
|
||
| import Button | ||
| import Html exposing (Html) | ||
| import Html.Attributes as Attributes | ||
| import Html.Events as Events | ||
| import Html.Extra | ||
| import Icon | ||
| import Types exposing (Tag) | ||
|
|
||
|
|
||
| type alias FormModel msg = | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why call it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In fact, why call it
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point, maybe |
||
| { newTag : Maybe Tag | ||
| , formErrorMessage : Maybe String | ||
| , newTagNameInputMsg : String -> msg | ||
| , newTagValueInputMsg : String -> msg | ||
| , addTagMsg : msg | ||
| , cancelMsg : msg | ||
| } | ||
|
|
||
|
|
||
| requiredAsteriskView : Html msg | ||
| requiredAsteriskView = | ||
| Html.span [ Attributes.class "text-red-500", Attributes.attribute "aria-hidden" "true" ] [ Html.text "\u{200A}*\u{00A0}" ] | ||
|
|
||
|
|
||
| errorMessageView : String -> Html msg | ||
| errorMessageView errorMessage = | ||
| Html.div [ Attributes.class "p-3 bg-rose-100 border border-rose-400 text-rose-700 rounded" ] [ Html.text errorMessage ] | ||
|
|
||
|
|
||
| view : FormModel msg -> Html msg | ||
| view { newTag, formErrorMessage, newTagNameInputMsg, newTagValueInputMsg, addTagMsg, cancelMsg } = | ||
| Html.section [ Attributes.class "w-full max-w-lg mx-auto flex flex-col gap-4 p-4 border border-gray-200 rounded" ] | ||
| [ Html.h3 [ Attributes.class "text-lg font-bold" ] [ Html.text "Add a new tag" ] | ||
| , Html.p [ Attributes.class "text-sm text-gray-600" ] [ Html.text "* Required fields" ] | ||
| , Html.Extra.viewMaybe errorMessageView formErrorMessage | ||
| , Html.form | ||
| [ Attributes.id "add-tag-form" | ||
| , Attributes.class "flex flex-col gap-4" | ||
| , Events.onSubmit addTagMsg | ||
| ] | ||
| [ Html.label [ Attributes.class "flex flex-col gap-1 text-sm font-medium" ] | ||
| [ Html.p [] | ||
| [ Html.text "Tag name" | ||
| , requiredAsteriskView | ||
| ] | ||
| , Html.input | ||
| [ Attributes.type_ "text" | ||
| , Attributes.id "tag-name-input" | ||
| , Attributes.required True | ||
| , Attributes.placeholder "Enter name, e.g. 'Powered by'" | ||
| , Attributes.maxlength 32 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why have you choose the limit to be 32?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| , Attributes.attribute "aria-describedby" "tag-name-max-length" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| , Attributes.class "border border-gray-300 rounded px-3 py-2" | ||
| , newTag | ||
| |> Maybe.map (\{ name } -> name) | ||
| |> Maybe.withDefault "" | ||
| |> Attributes.value | ||
| , Events.onInput newTagNameInputMsg | ||
| ] | ||
| [] | ||
| , Html.p [ Attributes.id "tag-name-max-length", Attributes.class "text-xs text-gray-600" ] [ Html.text "Maximum length is 32 characters." ] | ||
| ] | ||
| , Html.label [ Attributes.class "flex flex-col gap-1 text-sm font-medium" ] | ||
| [ Html.text "Tag value" | ||
| , Html.input | ||
| [ Attributes.type_ "text" | ||
| , Attributes.id "tag-value-input" | ||
| , Attributes.placeholder "Enter value, e.g. 'elm'" | ||
| , Attributes.class "border border-gray-300 rounded px-3 py-2" | ||
| , newTag | ||
| |> Maybe.andThen .value | ||
| |> Maybe.withDefault "" | ||
| |> Attributes.value | ||
| , Events.onInput newTagValueInputMsg | ||
| ] | ||
| [] | ||
| ] | ||
| , Html.div [ Attributes.class "flex flex-col sm:flex-row sm:items-center sm:justify-end gap-3" ] | ||
| [ Button.new | ||
| { class = "text-gray-900 font-bold py-2 px-4 rounded border border-gray-900" | ||
| , label = "Cancel" | ||
| } | ||
| |> Button.withAriaLabel "Cancel adding a new tag" | ||
| |> Button.withOnClick cancelMsg | ||
| |> Button.view | ||
| , Button.new | ||
| { class = "bg-gray-900 hover:bg-gray-700 text-white font-bold py-2 px-4 rounded border border-gray-800 transition" | ||
| , label = "Add a new tag" | ||
| } | ||
| |> Button.withAriaLabel "Confirm adding a new tag" | ||
| |> Button.withType "submit" | ||
| |> Button.withIconLeft (Just Icon.saveIcon) | ||
| |> Button.view | ||
| ] | ||
| ] | ||
| ] | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,91 @@ | ||
| module Button exposing (new, view, withAriaLabel, withFormId, withIconLeft, withIconRight, withId, withOnClick, withType) | ||
|
|
||
| import Html as Html exposing (Html) | ||
| import Html.Attributes as Attributes | ||
| import Html.Attributes.Extra | ||
| import Html.Events as Events | ||
| import Html.Extra | ||
|
|
||
|
|
||
| type Button msg | ||
| = Settings | ||
| { class : String | ||
| , label : String | ||
| , leftIcon : Maybe (Html Never) | ||
| , rightIcon : Maybe (Html Never) | ||
| , onClickMsg : Maybe msg | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I made it 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 |
||
| , buttonType : String | ||
| , additionalAttributes : List (Html.Attribute msg) | ||
| } | ||
|
|
||
|
|
||
| new : { class : String, label : String } -> Button msg | ||
| new { class, label } = | ||
| Settings | ||
| { class = class | ||
| , label = label | ||
| , leftIcon = Nothing | ||
| , rightIcon = Nothing | ||
| , onClickMsg = Nothing | ||
| , buttonType = "button" | ||
| , additionalAttributes = [] | ||
| } | ||
|
|
||
|
|
||
| withFormId : String -> Button msg -> Button msg | ||
| withFormId formId (Settings model) = | ||
| Settings | ||
| { model | ||
| | additionalAttributes = | ||
| Attributes.form formId :: model.additionalAttributes | ||
| } | ||
|
|
||
|
|
||
| withAriaLabel : String -> Button msg -> Button msg | ||
| withAriaLabel label (Settings model) = | ||
| Settings | ||
| { model | ||
| | additionalAttributes = | ||
| Attributes.attribute "aria-label" label :: model.additionalAttributes | ||
| } | ||
|
|
||
|
|
||
| withId : String -> Button msg -> Button msg | ||
| withId id (Settings model) = | ||
| Settings { model | additionalAttributes = Attributes.id id :: model.additionalAttributes } | ||
|
|
||
|
|
||
| withOnClick : msg -> Button msg -> Button msg | ||
| withOnClick msg (Settings model) = | ||
| Settings { model | onClickMsg = Just msg } | ||
|
|
||
|
|
||
| withIconLeft : Maybe (Html Never) -> Button msg -> Button msg | ||
| withIconLeft icon (Settings model) = | ||
| Settings { model | leftIcon = icon } | ||
|
|
||
|
|
||
| withIconRight : Maybe (Html Never) -> Button msg -> Button msg | ||
| withIconRight icon (Settings model) = | ||
| Settings { model | rightIcon = icon } | ||
|
|
||
|
|
||
| withType : String -> Button msg -> Button msg | ||
| withType buttonType (Settings model) = | ||
| Settings { model | buttonType = buttonType } | ||
|
|
||
|
|
||
| view : Button msg -> Html msg | ||
| view (Settings { class, buttonType, additionalAttributes, onClickMsg, leftIcon, rightIcon, label }) = | ||
| Html.button | ||
| ([ Attributes.class (class ++ " flex items-center justify-center gap-2") | ||
| , Attributes.type_ buttonType | ||
| , Html.Attributes.Extra.attributeMaybe (\msg -> Events.onClick msg) onClickMsg | ||
| ] | ||
| ++ additionalAttributes | ||
| ) | ||
| <| | ||
| [ Html.Extra.viewMaybe Html.Extra.static leftIcon | ||
| , Html.text label | ||
| , Html.Extra.viewMaybe Html.Extra.static rightIcon | ||
| ] | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,99 @@ | ||
| module Icon exposing (pencilIcon, saveIcon, trashIcon) | ||
|
|
||
| import Html exposing (Html) | ||
| import Html.Attributes as Attributes | ||
| import Svg as Svg | ||
| import Svg.Attributes as SvgAttr | ||
|
|
||
|
|
||
| saveIcon : Html Never | ||
| saveIcon = | ||
| Html.span [ Attributes.attribute "aria-hidden" "true" ] | ||
| [ Svg.svg | ||
| [ SvgAttr.width "24" | ||
| , SvgAttr.height "24" | ||
| , SvgAttr.viewBox "0 0 24 24" | ||
| ] | ||
| [ Svg.g | ||
| [ SvgAttr.fill "none" | ||
| , SvgAttr.stroke "currentColor" | ||
| , SvgAttr.strokeLinecap "round" | ||
| , SvgAttr.strokeLinejoin "round" | ||
| , SvgAttr.strokeWidth "2" | ||
| ] | ||
| [ Svg.path | ||
| [ SvgAttr.d "M3 12c0 -4.97 4.03 -9 9 -9c4.97 0 9 4.03 9 9c0 4.97 -4.03 9 -9 9c-4.97 0 -9 -4.03 -9 -9Z" | ||
| ] | ||
| [] | ||
| , Svg.path | ||
| [ SvgAttr.d "M8 12l3 3l5 -5" | ||
| ] | ||
| [] | ||
| ] | ||
| ] | ||
| ] | ||
|
|
||
|
|
||
| pencilIcon : Html Never | ||
| pencilIcon = | ||
| Html.span [ Attributes.attribute "aria-hidden" "true" ] | ||
| [ Svg.svg | ||
| [ SvgAttr.width "24" | ||
| , SvgAttr.height "24" | ||
| , SvgAttr.viewBox "0 0 24 24" | ||
| ] | ||
| [ Svg.g | ||
| [ SvgAttr.fill "none" | ||
| , SvgAttr.stroke "currentColor" | ||
| , SvgAttr.strokeLinecap "round" | ||
| , SvgAttr.strokeLinejoin "round" | ||
| , SvgAttr.strokeWidth "2" | ||
| ] | ||
| [ Svg.path | ||
| [ SvgAttr.d "M3 21l2 -6l11 -11c1 -1 3 -1 4 0c1 1 1 3 0 4l-11 11l-6 2" | ||
| ] | ||
| [] | ||
| , Svg.path | ||
| [ SvgAttr.d "M15 5l4 4" | ||
| ] | ||
| [] | ||
| , Svg.path | ||
| [ SvgAttr.strokeWidth "1" | ||
| , SvgAttr.d "M6 15l3 3" | ||
| ] | ||
| [] | ||
| ] | ||
| ] | ||
| ] | ||
|
|
||
|
|
||
| trashIcon : Html Never | ||
| trashIcon = | ||
| Html.span [ Attributes.attribute "aria-hidden" "true" ] | ||
| [ Svg.svg | ||
| [ SvgAttr.width "24" | ||
| , SvgAttr.height "24" | ||
| , SvgAttr.viewBox "0 0 24 24" | ||
| ] | ||
| [ Svg.g | ||
| [ SvgAttr.fill "none" | ||
| , SvgAttr.stroke "currentColor" | ||
| , SvgAttr.strokeLinecap "round" | ||
| , SvgAttr.strokeLinejoin "round" | ||
| , SvgAttr.strokeWidth "2" | ||
| ] | ||
| [ Svg.path | ||
| [ SvgAttr.d "M12 20h5c0.5 0 1 -0.5 1 -1v-14M12 20h-5c-0.5 0 -1 -0.5 -1 -1v-14" | ||
| ] | ||
| [] | ||
| , Svg.path | ||
| [ SvgAttr.d "M4 5h16" | ||
| ] | ||
| [] | ||
| , Svg.path | ||
| [ SvgAttr.d "M10 4h4M10 9v7M14 9v7" | ||
| ] | ||
| [] | ||
| ] | ||
| ] | ||
| ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can extend the setup to your liking :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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