From 7acdda40de2acdf55796e0bf5ea5dc8eec5a80f7 Mon Sep 17 00:00:00 2001 From: charbelrami Date: Mon, 11 Sep 2023 11:51:32 -0300 Subject: [PATCH 1/6] add helpfully disabled support for Button --- component-catalog/src/Examples/Button.elm | 8 ++++++++ src/Nri/Ui/Button/V10.elm | 24 +++++++++++++++++++++-- 2 files changed, 30 insertions(+), 2 deletions(-) diff --git a/component-catalog/src/Examples/Button.elm b/component-catalog/src/Examples/Button.elm index b1361a57..b6872d5e 100644 --- a/component-catalog/src/Examples/Button.elm +++ b/component-catalog/src/Examples/Button.elm @@ -323,6 +323,14 @@ viewButtonExamples ellieLinkConfig state = , track = ShowItWorked "ButtonExample" "linkExternalWithTracking clicked" } ] + , form + [ css [ Css.marginTop Spacing.verticalSpacerPx ] + ] + [ Button.button "Submit" + [ Button.submit + , Button.disabled + ] + ] ] |> div [] diff --git a/src/Nri/Ui/Button/V10.elm b/src/Nri/Ui/Button/V10.elm index 1e1dea89..190279c0 100644 --- a/src/Nri/Ui/Button/V10.elm +++ b/src/Nri/Ui/Button/V10.elm @@ -758,8 +758,27 @@ renderButton ((ButtonOrLink config) as button_) = Nri.Ui.styled Html.button (styledName "customButton") (buttonStyles config) - (ClickableAttributes.toButtonAttributes config.clickableAttributes - { disabled = isDisabled config.state } + (ExtraAttributes.includeIf config.clickableAttributes.opensModal + (Attributes.attribute "aria-haspopup" "true") + :: (if isDisabled config.state then + Aria.disabled True + :: (if config.clickableAttributes.buttonType == "submit" then + case config.state of + Disabled -> + [ Attributes.type_ "button" ] + + _ -> + [ Attributes.type_ "submit" ] + + else + [ Attributes.type_ config.clickableAttributes.buttonType ] + ) + + else + [ Attributes.type_ config.clickableAttributes.buttonType + , ExtraAttributes.maybe Events.onClick config.clickableAttributes.onClick + ] + ) ++ Attributes.class FocusRing.customClass :: ExtraAttributes.maybe (Just >> Aria.pressed) config.pressed :: config.customAttributes @@ -1229,6 +1248,7 @@ applyColorStyle colorPalette = [ Css.color colorPalette.hoverText , Css.backgroundColor colorPalette.hoverBackground , Css.disabled [ Css.backgroundColor colorPalette.background ] + , Css.Global.withAttribute "aria-disabled=true" [ Css.backgroundColor colorPalette.background ] ] , Css.visited [ Css.color colorPalette.text ] ] From b0bcdf28d6a8ef8d67974071c2fd062b28bc701a Mon Sep 17 00:00:00 2001 From: charbelrami Date: Mon, 11 Sep 2023 13:14:06 -0300 Subject: [PATCH 2/6] add example of a disabled button with tooltip --- component-catalog/src/Examples/Button.elm | 25 +++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/component-catalog/src/Examples/Button.elm b/component-catalog/src/Examples/Button.elm index b6872d5e..a27ee553 100644 --- a/component-catalog/src/Examples/Button.elm +++ b/component-catalog/src/Examples/Button.elm @@ -6,6 +6,7 @@ module Examples.Button exposing (Msg, State, example) -} +import Accessibility.Styled.Aria as Aria import Accessibility.Styled.Key as Key import Category exposing (Category(..)) import Code @@ -24,6 +25,7 @@ import Nri.Ui.Colors.V1 as Colors import Nri.Ui.Heading.V3 as Heading import Nri.Ui.Message.V4 as Message import Nri.Ui.Spacing.V1 as Spacing +import Nri.Ui.Tooltip.V3 as Tooltip import Nri.Ui.UiIcon.V1 as UiIcon import Routes import Set exposing (Set) @@ -270,6 +272,11 @@ initDebugControls = ) +tooltipId : String +tooltipId = + "tooltip" + + viewButtonExamples : EllieLink.Config -> State -> Html Msg viewButtonExamples ellieLinkConfig state = let @@ -324,13 +331,27 @@ viewButtonExamples ellieLinkConfig state = } ] , form - [ css [ Css.marginTop Spacing.verticalSpacerPx ] - ] + [ css [ Css.marginTop Spacing.verticalSpacerPx ] ] [ Button.button "Submit" [ Button.submit , Button.disabled ] ] + , div [ css [ Css.marginTop Spacing.verticalSpacerPx ] ] + [ Tooltip.view + { trigger = + \attrs -> + Button.button "Disabled with tooltip" + [ Button.disabled + , Button.custom (Aria.describedBy [ tooltipId ] :: attrs) + ] + , id = tooltipId + } + [ Tooltip.open True + , Tooltip.paragraph "The quick brown fox jumps over the lazy dog." + , Tooltip.onRight + ] + ] ] |> div [] From 4a65db396a579c69f6e503bedf4f170328a54f5e Mon Sep 17 00:00:00 2001 From: charbelrami Date: Mon, 11 Sep 2023 15:21:22 -0300 Subject: [PATCH 3/6] add docs and spec for helpfully disabled button --- src/Nri/Ui/Button/V10.elm | 5 ++- tests/Spec/Nri/Ui/Button.elm | 60 ++++++++++++++++++++++++++++++++++++ 2 files changed, 64 insertions(+), 1 deletion(-) diff --git a/src/Nri/Ui/Button/V10.elm b/src/Nri/Ui/Button/V10.elm index 190279c0..c0526d3c 100644 --- a/src/Nri/Ui/Button/V10.elm +++ b/src/Nri/Ui/Button/V10.elm @@ -39,7 +39,10 @@ The next version of `Button` should also remove `delete` and `toggleButton` - adds `tertiary` style - adds `submit` and `opensModal` - adds `secondaryDanger` style - - marked toggleButton as deprecated and added toggleButtonPressed attribute + - marks toggleButton as deprecated and adds toggleButtonPressed attribute + - replaces the `disabled` attribute with `aria-disabled="true"` + - removes click handler from disabled buttons + - prevents default behavior for disabled submit buttons by setting `type="button"` # Changes from V9: diff --git a/tests/Spec/Nri/Ui/Button.elm b/tests/Spec/Nri/Ui/Button.elm index 2d1ff5d1..1c6a8e85 100644 --- a/tests/Spec/Nri/Ui/Button.elm +++ b/tests/Spec/Nri/Ui/Button.elm @@ -1,17 +1,20 @@ module Spec.Nri.Ui.Button exposing (spec) import Accessibility.Aria as Aria +import Expect exposing (Expectation) import Html.Styled exposing (Html, toUnstyled) import Nri.Ui.Button.V10 as Button import ProgramTest exposing (..) import Test exposing (..) import Test.Html.Selector exposing (..) +import Test.Runner spec : Test spec = describe "Nri.Ui.Button.V10" [ describe "toggleButtonPressed" toggleButtonPressed + , describe "helpfullyDisabledButton" helpfullyDisabledButton ] @@ -35,6 +38,63 @@ toggleButtonPressed = ] +expectFailure : String -> Expectation -> Expectation +expectFailure expectedDescriptionSubstring expectation = + case Test.Runner.getFailureReason expectation of + Nothing -> + Expect.fail "Expected a failure, but there was none." + + Just reason -> + String.contains expectedDescriptionSubstring reason.description + |> Expect.equal True + + +helpfullyDisabledButton : List Test +helpfullyDisabledButton = + [ test "does not have `aria-disabled=\"true\" when not disabled" <| + \() -> + program () + (\_ -> + Button.button "Italic" + [] + ) + |> ensureViewHasNot [ attribute (Aria.disabled True) ] + |> done + , test "has `aria-disabled=\"true\" when disabled" <| + \() -> + program () + (\_ -> + Button.button "Italic" + [ Button.disabled + ] + ) + |> ensureViewHas [ attribute (Aria.disabled True) ] + |> done + , test "is clickable when not disabled" <| + \() -> + program () + (\_ -> + Button.button "Italic" + [ Button.onClick () + ] + ) + |> clickButton "Italic" + |> done + , test "is not clickable when disabled" <| + \() -> + program () + (\_ -> + Button.button "Italic" + [ Button.onClick () + , Button.disabled + ] + ) + |> clickButton "Italic" + |> done + |> expectFailure "Event.expectEvent: I found a node, but it does not listen for \"click\" events like I expected it would." + ] + + program : model -> (model -> Html model) -> ProgramTest model model () program init view = ProgramTest.createSandbox From 04b84b2ecbc61bf42a87b2d144dfb522962cc5df Mon Sep 17 00:00:00 2001 From: charbelrami Date: Mon, 11 Sep 2023 15:30:56 -0300 Subject: [PATCH 4/6] use expectFailure as a helper --- tests/Spec/Helpers.elm | 13 +++++++++++++ tests/Spec/Nri/Ui/Button.elm | 14 +------------- 2 files changed, 14 insertions(+), 13 deletions(-) diff --git a/tests/Spec/Helpers.elm b/tests/Spec/Helpers.elm index f5a22a2e..e82790d5 100644 --- a/tests/Spec/Helpers.elm +++ b/tests/Spec/Helpers.elm @@ -1,9 +1,22 @@ module Spec.Helpers exposing (..) +import Expect exposing (Expectation) import Html.Attributes as Attributes import Test.Html.Selector as Selector +import Test.Runner nriDescription : String -> Selector.Selector nriDescription desc = Selector.attribute (Attributes.attribute "data-nri-description" desc) + + +expectFailure : String -> Expectation -> Expectation +expectFailure expectedDescriptionSubstring expectation = + case Test.Runner.getFailureReason expectation of + Nothing -> + Expect.fail "Expected a failure, but there was none." + + Just reason -> + String.contains expectedDescriptionSubstring reason.description + |> Expect.equal True diff --git a/tests/Spec/Nri/Ui/Button.elm b/tests/Spec/Nri/Ui/Button.elm index 1c6a8e85..8bdbc09e 100644 --- a/tests/Spec/Nri/Ui/Button.elm +++ b/tests/Spec/Nri/Ui/Button.elm @@ -1,13 +1,12 @@ module Spec.Nri.Ui.Button exposing (spec) import Accessibility.Aria as Aria -import Expect exposing (Expectation) import Html.Styled exposing (Html, toUnstyled) import Nri.Ui.Button.V10 as Button import ProgramTest exposing (..) +import Spec.Helpers exposing (expectFailure) import Test exposing (..) import Test.Html.Selector exposing (..) -import Test.Runner spec : Test @@ -38,17 +37,6 @@ toggleButtonPressed = ] -expectFailure : String -> Expectation -> Expectation -expectFailure expectedDescriptionSubstring expectation = - case Test.Runner.getFailureReason expectation of - Nothing -> - Expect.fail "Expected a failure, but there was none." - - Just reason -> - String.contains expectedDescriptionSubstring reason.description - |> Expect.equal True - - helpfullyDisabledButton : List Test helpfullyDisabledButton = [ test "does not have `aria-disabled=\"true\" when not disabled" <| From f3c6aed490aef17ce29f609b7faafc7a88a275fc Mon Sep 17 00:00:00 2001 From: charbelrami Date: Mon, 11 Sep 2023 18:09:44 -0300 Subject: [PATCH 5/6] remove unnecessary check --- src/Nri/Ui/Button/V10.elm | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/src/Nri/Ui/Button/V10.elm b/src/Nri/Ui/Button/V10.elm index c0526d3c..0d311ad3 100644 --- a/src/Nri/Ui/Button/V10.elm +++ b/src/Nri/Ui/Button/V10.elm @@ -766,12 +766,7 @@ renderButton ((ButtonOrLink config) as button_) = :: (if isDisabled config.state then Aria.disabled True :: (if config.clickableAttributes.buttonType == "submit" then - case config.state of - Disabled -> - [ Attributes.type_ "button" ] - - _ -> - [ Attributes.type_ "submit" ] + [ Attributes.type_ "button" ] else [ Attributes.type_ config.clickableAttributes.buttonType ] From 923f104ea6200af64b02141b482c565a7d161d86 Mon Sep 17 00:00:00 2001 From: charbelrami Date: Mon, 11 Sep 2023 18:17:59 -0300 Subject: [PATCH 6/6] add missing button example headings --- component-catalog/src/Examples/Button.elm | 63 +++++++++++++++++------ 1 file changed, 46 insertions(+), 17 deletions(-) diff --git a/component-catalog/src/Examples/Button.elm b/component-catalog/src/Examples/Button.elm index a27ee553..437a1e2b 100644 --- a/component-catalog/src/Examples/Button.elm +++ b/component-catalog/src/Examples/Button.elm @@ -313,15 +313,28 @@ viewButtonExamples ellieLinkConfig state = } , Heading.h2 [ Heading.plaintext "Interactive example" - , Heading.css [ Css.margin2 Spacing.verticalSpacerPx Css.zero ] + , Heading.css + [ Css.margin2 Spacing.verticalSpacerPx Css.zero + , Css.marginBottom (Css.px 10) + ] ] , viewCustomizableExample model , Heading.h2 [ Heading.plaintext "Non-interactive examples" - , Heading.css [ Css.marginTop Spacing.verticalSpacerPx ] + , Heading.css + [ Css.marginTop Spacing.verticalSpacerPx + , Css.marginBottom (Css.px 10) + ] ] , buttonsTable , toggleButtons state.pressedToggleButtons + , Heading.h2 + [ Heading.plaintext "Link with tracking" + , Heading.css + [ Css.marginTop Spacing.verticalSpacerPx + , Css.marginBottom (Css.px 10) + ] + ] , Button.link "linkExternalWithTracking" [ Button.unboundedWidth , Button.secondary @@ -330,28 +343,41 @@ viewButtonExamples ellieLinkConfig state = , track = ShowItWorked "ButtonExample" "linkExternalWithTracking clicked" } ] + , Heading.h2 + [ Heading.plaintext "Disabled form submit button" + , Heading.css + [ Css.marginTop Spacing.verticalSpacerPx + , Css.marginBottom (Css.px 10) + ] + ] , form - [ css [ Css.marginTop Spacing.verticalSpacerPx ] ] + [] [ Button.button "Submit" [ Button.submit , Button.disabled ] ] - , div [ css [ Css.marginTop Spacing.verticalSpacerPx ] ] - [ Tooltip.view - { trigger = - \attrs -> - Button.button "Disabled with tooltip" - [ Button.disabled - , Button.custom (Aria.describedBy [ tooltipId ] :: attrs) - ] - , id = tooltipId - } - [ Tooltip.open True - , Tooltip.paragraph "The quick brown fox jumps over the lazy dog." - , Tooltip.onRight + , Heading.h2 + [ Heading.plaintext "Disabled button with tooltip" + , Heading.css + [ Css.marginTop Spacing.verticalSpacerPx + , Css.marginBottom (Css.px 10) ] ] + , Tooltip.view + { trigger = + \attrs -> + Button.button "Save" + [ Button.disabled + , Button.custom (Aria.describedBy [ tooltipId ] :: attrs) + ] + , id = tooltipId + } + [ Tooltip.open True + , Tooltip.paragraph "Reasons why you can't save" + , Tooltip.onRight + , Tooltip.fitToContent + ] ] |> div [] @@ -450,7 +476,10 @@ toggleButtons pressedToggleButtons = div [] [ Heading.h2 [ Heading.plaintext "Button toggle" - , Heading.css [ Css.marginTop Spacing.verticalSpacerPx ] + , Heading.css + [ Css.marginTop Spacing.verticalSpacerPx + , Css.marginBottom (Css.px 10) + ] ] , div [ css [ Css.displayFlex, Css.marginBottom (Css.px 20) ] ] [ Button.button "5"