From 1c43f068088f8b3cb93cc183d49107a7a25bc41e Mon Sep 17 00:00:00 2001 From: Ju Liu Date: Wed, 27 Nov 2019 15:00:20 +0000 Subject: [PATCH 01/14] Make modal ignore horizontal overflow --- src/Nri/Ui/Modal/V8.elm | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Nri/Ui/Modal/V8.elm b/src/Nri/Ui/Modal/V8.elm index 4ce9c936..14b71c4c 100644 --- a/src/Nri/Ui/Modal/V8.elm +++ b/src/Nri/Ui/Modal/V8.elm @@ -344,6 +344,7 @@ viewInnerContent children visibleTitle visibleFooter = [ div [ css [ Css.overflowY Css.auto + , Css.overflowX Css.hidden , Css.minHeight (Css.px 150) , Css.maxHeight (Css.calc (Css.vh 100) From 490395d4a78f1f4c17a33d752574e92b50a29243 Mon Sep 17 00:00:00 2001 From: brookeangel Date: Mon, 2 Dec 2019 14:53:49 -0800 Subject: [PATCH 02/14] convert styles to elm css --- src/Nri/Ui/Tooltip/V1.elm | 46 +++++++++++++++++++-------------------- 1 file changed, 23 insertions(+), 23 deletions(-) diff --git a/src/Nri/Ui/Tooltip/V1.elm b/src/Nri/Ui/Tooltip/V1.elm index dcf747d7..7eee7be3 100644 --- a/src/Nri/Ui/Tooltip/V1.elm +++ b/src/Nri/Ui/Tooltip/V1.elm @@ -352,6 +352,7 @@ viewIf viewFn condition = viewTooltip : Maybe String -> Tooltip msg -> Html msg viewTooltip maybeTooltipId (Tooltip config) = Html.div (containerPositioningForArrowPosition config.position) + Html.div [ css <| containerPositioningForArrowPosition config.position ] [ Html.div ([ css ([ Css.borderRadius (Css.px 8) @@ -426,33 +427,32 @@ tooltipColor = {-| This returns an absolute positioning style attribute for the popout container for a given arrow position. -} -containerPositioningForArrowPosition : Position -> List (Attribute msg) +containerPositioningForArrowPosition : Position -> List Style containerPositioningForArrowPosition arrowPosition = - List.map (\( k, v ) -> Attributes.style k v) <| - case arrowPosition of - OnTop -> - [ ( "left", "50%" ) - , ( "top", "calc(-" ++ String.fromFloat arrowSize ++ "px - 2px)" ) - , ( "position", "absolute" ) - ] + case arrowPosition of + OnTop -> + [ Css.left (Css.pct 50) + , Css.top (Css.calc (Css.px (negate arrowSize)) Css.minus (Css.px 2)) + , Css.position Css.absolute + ] - OnBottom -> - [ ( "left", "50%" ) - , ( "bottom", "calc(-" ++ String.fromFloat arrowSize ++ "px - 2px)" ) - , ( "position", "absolute" ) - ] + OnBottom -> + [ Css.left (Css.pct 50) + , Css.bottom (Css.calc (Css.px (negate arrowSize)) Css.minus (Css.px 2)) + , Css.position Css.absolute + ] - OnLeft -> - [ ( "top", "50%" ) - , ( "left", "calc(-" ++ String.fromFloat arrowSize ++ "px - 2px)" ) - , ( "position", "absolute" ) - ] + OnLeft -> + [ Css.top (Css.pct 50) + , Css.left (Css.calc (Css.px (negate arrowSize)) Css.minus (Css.px 2)) + , Css.position Css.absolute + ] - OnRight -> - [ ( "top", "50%" ) - , ( "right", "calc(-" ++ String.fromFloat arrowSize ++ "px - 2px)" ) - , ( "position", "absolute" ) - ] + OnRight -> + [ Css.top (Css.pct 50) + , Css.right (Css.calc (Css.px (negate arrowSize)) Css.minus (Css.px 2)) + , Css.position Css.absolute + ] pointerBox : Position -> Attribute msg From d2a9200e5b90e25f8029e0c95a997f3c568a08fc Mon Sep 17 00:00:00 2001 From: brookeangel Date: Mon, 2 Dec 2019 15:54:59 -0800 Subject: [PATCH 03/14] give toggletip a "hover bridge" --- src/Nri/Ui/Tooltip/V1.elm | 46 +++++++++++++++-------------- styleguide-app/Examples/Tooltip.elm | 35 +++++++++++++++++++--- 2 files changed, 55 insertions(+), 26 deletions(-) diff --git a/src/Nri/Ui/Tooltip/V1.elm b/src/Nri/Ui/Tooltip/V1.elm index 7eee7be3..745b151f 100644 --- a/src/Nri/Ui/Tooltip/V1.elm +++ b/src/Nri/Ui/Tooltip/V1.elm @@ -223,7 +223,7 @@ auxillaryDescription = {-| Supplementary information triggered by a "?" icon -A toggle tip is always triggered by a click. +A toggle tip is always triggered by a hover (or focus, for keyboard users) -} toggleTip : @@ -242,26 +242,29 @@ toggleTip { isOpen, onTrigger, extraButtonAttrs, label } tooltip_ = [ Html.button ([ Widget.label label , css - ([ Css.width (Css.px 20) - , Css.height (Css.px 20) - , Css.color Colors.azure - ] - ++ buttonStyleOverrides + (buttonStyleOverrides + ++ [ Css.padding (Css.px 10) -- This is needed to provide a "hover bridge", so the tooltip won't close when trying to click a link + ] ) - , EventExtras.onClickStopPropagation (onTrigger True) - , Events.onBlur (onTrigger False) ] + ++ eventsForTrigger OnHover onTrigger ++ extraButtonAttrs ) - [ iconHelp ] - , viewIf (\_ -> viewCloseTooltipOverlay (onTrigger False)) isOpen - , Html.span - [ -- This adds aria-live polite & also aria-live atomic, so our screen readers are alerted when content appears - Role.status - ] - [ -- Popout is rendered after the overlay, to allow client code to give it - -- priority when clicking by setting its position - viewIf (\_ -> viewTooltip Nothing tooltip_) isOpen + [ Html.div + [ css + [ Css.position Css.relative + , Css.width (Css.px 20) + , Css.height (Css.px 20) + , Css.color Colors.azure + ] + ] + [ iconHelp + , Html.span + [ -- This adds aria-live polite & also aria-live atomic, so our screen readers are alerted when content appears + Role.status + ] + [ viewIf (\_ -> viewTooltip Nothing OnHover tooltip_) isOpen ] + ] ] ] @@ -333,7 +336,7 @@ viewTooltip_ purpose { trigger, triggerHtml, onTrigger, isOpen, id, extraButtonA -- Popout is rendered after the overlay, to allow client code to give it -- priority when clicking by setting its position - , viewIf (\_ -> viewTooltip (Just id) tooltip_) isOpen + , viewIf (\_ -> viewTooltip (Just id) trigger tooltip_) isOpen ] @@ -349,10 +352,9 @@ viewIf viewFn condition = Html.text "" -viewTooltip : Maybe String -> Tooltip msg -> Html msg -viewTooltip maybeTooltipId (Tooltip config) = - Html.div (containerPositioningForArrowPosition config.position) - Html.div [ css <| containerPositioningForArrowPosition config.position ] +viewTooltip : Maybe String -> Trigger -> Tooltip msg -> Html msg +viewTooltip maybeTooltipId trigger (Tooltip config) = + Html.div [ css (containerPositioningForArrowPosition config.position) ] [ Html.div ([ css ([ Css.borderRadius (Css.px 8) diff --git a/styleguide-app/Examples/Tooltip.elm b/styleguide-app/Examples/Tooltip.elm index 9a7ac071..438faee4 100644 --- a/styleguide-app/Examples/Tooltip.elm +++ b/styleguide-app/Examples/Tooltip.elm @@ -19,7 +19,10 @@ type TooltipType = PrimaryLabelOnClick | PrimaryLabelOnHover | AuxillaryDescription - | ToggleTip + | ToggleTipTop + | ToggleTipRight + | ToggleTipBottom + | ToggleTipLeft type alias State = @@ -97,10 +100,34 @@ example msg model = , Html.br [ css [ Css.marginBottom (Css.px 20) ] ] , Heading.h3 [] [ Html.text "toggleTip" ] , Text.smallBody [ Html.text "A Toggle Tip is triggered by the \"?\" icon and provides supplemental information for the page." ] - , Tooltip.tooltip [ Html.text "Tooltip" ] + , Tooltip.tooltip [ Html.text "Tooltip On Top" ] |> Tooltip.toggleTip - { onTrigger = ToggleTooltip ToggleTip >> msg - , isOpen = model.openTooltip == Just ToggleTip + { onTrigger = ToggleTooltip ToggleTipTop >> msg + , isOpen = model.openTooltip == Just ToggleTipTop + , label = "More info" + , extraButtonAttrs = [] + } + , Tooltip.tooltip [ Html.text "Tooltip On Left" ] + |> Tooltip.withPosition Tooltip.OnLeft + |> Tooltip.toggleTip + { onTrigger = ToggleTooltip ToggleTipLeft >> msg + , isOpen = model.openTooltip == Just ToggleTipLeft + , label = "More info" + , extraButtonAttrs = [] + } + , Tooltip.tooltip [ Html.text "Tooltip On Right" ] + |> Tooltip.withPosition Tooltip.OnRight + |> Tooltip.toggleTip + { onTrigger = ToggleTooltip ToggleTipRight >> msg + , isOpen = model.openTooltip == Just ToggleTipRight + , label = "More info" + , extraButtonAttrs = [] + } + , Tooltip.tooltip [ Html.text "Tooltip On Bottom" ] + |> Tooltip.withPosition Tooltip.OnBottom + |> Tooltip.toggleTip + { onTrigger = ToggleTooltip ToggleTipBottom >> msg + , isOpen = model.openTooltip == Just ToggleTipBottom , label = "More info" , extraButtonAttrs = [] } From 0686fb3b2319a6dd69502c838b0df22b9f7ef9d3 Mon Sep 17 00:00:00 2001 From: brookeangel Date: Mon, 2 Dec 2019 16:13:14 -0800 Subject: [PATCH 04/14] add links for clicks --- styleguide-app/Examples/Tooltip.elm | 30 ++++++++++++++++++++++++----- 1 file changed, 25 insertions(+), 5 deletions(-) diff --git a/styleguide-app/Examples/Tooltip.elm b/styleguide-app/Examples/Tooltip.elm index 438faee4..754858d7 100644 --- a/styleguide-app/Examples/Tooltip.elm +++ b/styleguide-app/Examples/Tooltip.elm @@ -8,7 +8,7 @@ module Examples.Tooltip exposing (example, init, update, State, Msg) import Accessibility.Styled as Html import Css -import Html.Styled.Attributes exposing (css) +import Html.Styled.Attributes exposing (css, href) import ModuleExample as ModuleExample exposing (Category(..), ModuleExample) import Nri.Ui.Heading.V2 as Heading import Nri.Ui.Text.V3 as Text @@ -100,14 +100,24 @@ example msg model = , Html.br [ css [ Css.marginBottom (Css.px 20) ] ] , Heading.h3 [] [ Html.text "toggleTip" ] , Text.smallBody [ Html.text "A Toggle Tip is triggered by the \"?\" icon and provides supplemental information for the page." ] - , Tooltip.tooltip [ Html.text "Tooltip On Top" ] + , Tooltip.tooltip + [ Html.text "Tooltip On Top! " + , Html.a + [ href "/" ] + [ Html.text "Links work!" ] + ] |> Tooltip.toggleTip { onTrigger = ToggleTooltip ToggleTipTop >> msg , isOpen = model.openTooltip == Just ToggleTipTop , label = "More info" , extraButtonAttrs = [] } - , Tooltip.tooltip [ Html.text "Tooltip On Left" ] + , Tooltip.tooltip + [ Html.text "Tooltip On Left! " + , Html.a + [ href "/" ] + [ Html.text "Links work!" ] + ] |> Tooltip.withPosition Tooltip.OnLeft |> Tooltip.toggleTip { onTrigger = ToggleTooltip ToggleTipLeft >> msg @@ -115,7 +125,12 @@ example msg model = , label = "More info" , extraButtonAttrs = [] } - , Tooltip.tooltip [ Html.text "Tooltip On Right" ] + , Tooltip.tooltip + [ Html.text "Tooltip On Right! " + , Html.a + [ href "/" ] + [ Html.text "Links work!" ] + ] |> Tooltip.withPosition Tooltip.OnRight |> Tooltip.toggleTip { onTrigger = ToggleTooltip ToggleTipRight >> msg @@ -123,7 +138,12 @@ example msg model = , label = "More info" , extraButtonAttrs = [] } - , Tooltip.tooltip [ Html.text "Tooltip On Bottom" ] + , Tooltip.tooltip + [ Html.text "Tooltip On Bottom! " + , Html.a + [ href "/" ] + [ Html.text "Links work!" ] + ] |> Tooltip.withPosition Tooltip.OnBottom |> Tooltip.toggleTip { onTrigger = ToggleTooltip ToggleTipBottom >> msg From 4ae52055e3acb2be668769186588d4a36a89ff36 Mon Sep 17 00:00:00 2001 From: brookeangel Date: Mon, 2 Dec 2019 16:13:26 -0800 Subject: [PATCH 05/14] fix test for toggletip --- tests/Spec/Nri/Ui/Tooltip/V1.elm | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/Spec/Nri/Ui/Tooltip/V1.elm b/tests/Spec/Nri/Ui/Tooltip/V1.elm index 07563392..9e38f1b9 100644 --- a/tests/Spec/Nri/Ui/Tooltip/V1.elm +++ b/tests/Spec/Nri/Ui/Tooltip/V1.elm @@ -37,7 +37,7 @@ spec : Test spec = describe "Nri.Ui.Tooltip.V1" [ describe "toggleTip" - [ test "Toggletip is available on click and hides on blur" <| + [ test "Toggletip is available on hover and hides on blur" <| \() -> ProgramTest.createSandbox { init = init @@ -52,7 +52,7 @@ spec = (Widget.label "More info") ] ) - Event.click + Event.mouseEnter |> ProgramTest.ensureViewHas [ Selector.text "Toggly" ] From daece85d38d20ac5f7f1e8ad1359bf503cae7ff1 Mon Sep 17 00:00:00 2001 From: brookeangel Date: Mon, 2 Dec 2019 16:17:15 -0800 Subject: [PATCH 06/14] suggest improvements for next version --- src/Nri/Ui/Tooltip/V1.elm | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/Nri/Ui/Tooltip/V1.elm b/src/Nri/Ui/Tooltip/V1.elm index 745b151f..4dc5c1c5 100644 --- a/src/Nri/Ui/Tooltip/V1.elm +++ b/src/Nri/Ui/Tooltip/V1.elm @@ -26,6 +26,14 @@ Example usage: } +## Suggested Improvements for V2 + + - The toggle tip does not currently manage focus correctly for keyboard users - if a + user tries to click on a link in the toggle tip, the tip will disappear as focus moves + to the next item in the page. This should be improved in the next release. + - Currently, only toggle tip supports links on hover - generalize this to all tooltips + + ## Tooltip Construction @docs Tooltip, tooltip From 4ae104bf9e59e10db387eb7341dc4239e5531a35 Mon Sep 17 00:00:00 2001 From: brookeangel Date: Tue, 3 Dec 2019 10:43:24 -0800 Subject: [PATCH 07/14] fix padding --- src/Nri/Ui/Tooltip/V1.elm | 69 +++++++++++++++++------- styleguide-app/Examples/Tooltip.elm | 84 ++++++++++++----------------- 2 files changed, 85 insertions(+), 68 deletions(-) diff --git a/src/Nri/Ui/Tooltip/V1.elm b/src/Nri/Ui/Tooltip/V1.elm index 4dc5c1c5..ea678c04 100644 --- a/src/Nri/Ui/Tooltip/V1.elm +++ b/src/Nri/Ui/Tooltip/V1.elm @@ -243,40 +243,73 @@ toggleTip : -> Tooltip msg -> Html msg toggleTip { isOpen, onTrigger, extraButtonAttrs, label } tooltip_ = + let + contentSize = + 20 + in Nri.Ui.styled Html.div "Nri-Ui-Tooltip-V1-ToggleTip" - tooltipContainerStyles + (tooltipContainerStyles + ++ [ -- Take up enough room within the document flow + Css.width (Css.px contentSize) + , Css.height (Css.px contentSize) + , Css.margin (Css.px 5) + ] + ) [] [ Html.button ([ Widget.label label - , css - (buttonStyleOverrides - ++ [ Css.padding (Css.px 10) -- This is needed to provide a "hover bridge", so the tooltip won't close when trying to click a link - ] - ) + , css buttonStyleOverrides ] ++ eventsForTrigger OnHover onTrigger ++ extraButtonAttrs ) - [ Html.div - [ css - [ Css.position Css.relative - , Css.width (Css.px 20) - , Css.height (Css.px 20) - , Css.color Colors.azure + [ hoverBridge contentSize + [ Html.div + [ css + [ Css.position Css.relative + , Css.width (Css.px contentSize) + , Css.height (Css.px contentSize) + , Css.color Colors.azure + ] ] - ] - [ iconHelp - , Html.span - [ -- This adds aria-live polite & also aria-live atomic, so our screen readers are alerted when content appears - Role.status + [ iconHelp + , Html.span + [ -- This adds aria-live polite & also aria-live atomic, so our screen readers are alerted when content appears + Role.status + ] + [ viewIf (\_ -> viewTooltip Nothing OnHover tooltip_) isOpen ] ] - [ viewIf (\_ -> viewTooltip Nothing OnHover tooltip_) isOpen ] ] ] ] +{-| Provides a "bridge" for the cursor to move from trigger content to tooltip, so the user can click on links, etc. + +Works by being larger than the trigger content & overlaying it, but is removed from the flow of the page (position: absolute), so that it looks ok visually. + +-} +hoverBridge : Float -> List (Html msg) -> Html msg +hoverBridge contentSize = + let + padding = + -- enough to cover the empty gap between tooltip and trigger content + 10 + in + Nri.Ui.styled Html.div + "tooltip-hover-bridge" + [ Css.boxSizing Css.borderBox + , Css.padding (Css.px padding) + , Css.width (Css.px <| contentSize + padding * 2) + , Css.height (Css.px <| contentSize + padding * 2) + , Css.position Css.absolute + , Css.top (Css.px <| negate padding) + , Css.left (Css.px <| negate padding) + ] + [] + + {-| Made with -} iconHelp : Svg msg diff --git a/styleguide-app/Examples/Tooltip.elm b/styleguide-app/Examples/Tooltip.elm index 754858d7..bcd9e1f4 100644 --- a/styleguide-app/Examples/Tooltip.elm +++ b/styleguide-app/Examples/Tooltip.elm @@ -11,7 +11,7 @@ import Css import Html.Styled.Attributes exposing (css, href) import ModuleExample as ModuleExample exposing (Category(..), ModuleExample) import Nri.Ui.Heading.V2 as Heading -import Nri.Ui.Text.V3 as Text +import Nri.Ui.Text.V4 as Text import Nri.Ui.Tooltip.V1 as Tooltip @@ -100,56 +100,40 @@ example msg model = , Html.br [ css [ Css.marginBottom (Css.px 20) ] ] , Heading.h3 [] [ Html.text "toggleTip" ] , Text.smallBody [ Html.text "A Toggle Tip is triggered by the \"?\" icon and provides supplemental information for the page." ] - , Tooltip.tooltip - [ Html.text "Tooltip On Top! " - , Html.a - [ href "/" ] - [ Html.text "Links work!" ] + , Html.div [ css [ Css.displayFlex, Css.alignItems Css.center ] ] + [ Tooltip.tooltip + [ Html.text "Tooltip On Top! " + , Html.a + [ href "/" ] + [ Html.text "Links work!" ] + ] + |> Tooltip.toggleTip + { onTrigger = ToggleTooltip ToggleTipTop >> msg + , isOpen = model.openTooltip == Just ToggleTipTop + , label = "More info" + , extraButtonAttrs = [] + } + , Text.mediumBody + [ Html.text "This toggletip will open on top" + ] ] - |> Tooltip.toggleTip - { onTrigger = ToggleTooltip ToggleTipTop >> msg - , isOpen = model.openTooltip == Just ToggleTipTop - , label = "More info" - , extraButtonAttrs = [] - } - , Tooltip.tooltip - [ Html.text "Tooltip On Left! " - , Html.a - [ href "/" ] - [ Html.text "Links work!" ] + , Html.div [ css [ Css.displayFlex, Css.alignItems Css.center ] ] + [ Tooltip.tooltip + [ Html.text "Tooltip On Left! " + , Html.a + [ href "/" ] + [ Html.text "Links work!" ] + ] + |> Tooltip.withPosition Tooltip.OnLeft + |> Tooltip.toggleTip + { onTrigger = ToggleTooltip ToggleTipLeft >> msg + , isOpen = model.openTooltip == Just ToggleTipLeft + , label = "More info" + , extraButtonAttrs = [] + } + , Text.mediumBody + [ Html.text "This toggletip will open on the left" + ] ] - |> Tooltip.withPosition Tooltip.OnLeft - |> Tooltip.toggleTip - { onTrigger = ToggleTooltip ToggleTipLeft >> msg - , isOpen = model.openTooltip == Just ToggleTipLeft - , label = "More info" - , extraButtonAttrs = [] - } - , Tooltip.tooltip - [ Html.text "Tooltip On Right! " - , Html.a - [ href "/" ] - [ Html.text "Links work!" ] - ] - |> Tooltip.withPosition Tooltip.OnRight - |> Tooltip.toggleTip - { onTrigger = ToggleTooltip ToggleTipRight >> msg - , isOpen = model.openTooltip == Just ToggleTipRight - , label = "More info" - , extraButtonAttrs = [] - } - , Tooltip.tooltip - [ Html.text "Tooltip On Bottom! " - , Html.a - [ href "/" ] - [ Html.text "Links work!" ] - ] - |> Tooltip.withPosition Tooltip.OnBottom - |> Tooltip.toggleTip - { onTrigger = ToggleTooltip ToggleTipBottom >> msg - , isOpen = model.openTooltip == Just ToggleTipBottom - , label = "More info" - , extraButtonAttrs = [] - } ] } From 381c8f6f5dd5ac9bcf8043cb6d22395fb729fd36 Mon Sep 17 00:00:00 2001 From: brookeangel Date: Tue, 3 Dec 2019 13:45:28 -0800 Subject: [PATCH 08/14] bump package to 7.14.1 --- elm.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/elm.json b/elm.json index 8cc9a2a6..7b882478 100644 --- a/elm.json +++ b/elm.json @@ -3,7 +3,7 @@ "name": "NoRedInk/noredink-ui", "summary": "UI Widgets we use at NRI", "license": "BSD-3-Clause", - "version": "7.14.0", + "version": "7.14.1", "exposed-modules": [ "Nri.Ui", "Nri.Ui.Accordion.V1", From f1636137b61046ddcb6fbc02af109ccd3acf3977 Mon Sep 17 00:00:00 2001 From: Brian Hicks Date: Wed, 4 Dec 2019 08:45:48 -0600 Subject: [PATCH 09/14] bump to 7.14.2 --- elm.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/elm.json b/elm.json index 7b882478..b39b6daf 100644 --- a/elm.json +++ b/elm.json @@ -3,7 +3,7 @@ "name": "NoRedInk/noredink-ui", "summary": "UI Widgets we use at NRI", "license": "BSD-3-Clause", - "version": "7.14.1", + "version": "7.14.2", "exposed-modules": [ "Nri.Ui", "Nri.Ui.Accordion.V1", From ccfee369acf3e960b3d1a7a6a52037fb8fc5eadc Mon Sep 17 00:00:00 2001 From: Brian Hicks Date: Wed, 4 Dec 2019 16:06:54 -0600 Subject: [PATCH 10/14] import the script from the monolith --- script/elm-deprecated-imports.py | 421 +++++++++++++++++++++++++++++++ 1 file changed, 421 insertions(+) create mode 100755 script/elm-deprecated-imports.py diff --git a/script/elm-deprecated-imports.py b/script/elm-deprecated-imports.py new file mode 100755 index 00000000..dbf29eef --- /dev/null +++ b/script/elm-deprecated-imports.py @@ -0,0 +1,421 @@ +#!/usr/bin/env python3 +""" +Make sure we don't use deprecated versions of Nri.Ui modules. We do this by only +allowing new imports for the highest version of every module. We keep an +artifact with old imports which haven't been removed yet, and are temporarily +allowed. See the subcommands of this script for managing this artifact! +""" + +import argparse +from collections import defaultdict +import csv +import json +import os +import os.path +import re +import sys +import textwrap + + +class ElmJson: + """parse elm.json for things we will need to determine version usage""" + + def __init__(self, path): + with open(path, "r") as fh: + self.data = json.load(fh) + + self.elm_version = self.data["elm-version"] + self.source_directories = self.data["source-directories"] + + def package_version(self, package): + return self.data["dependencies"]["direct"].get( + package, self.data["dependencies"]["indirect"].get(package, None) + ) + + +class NriUiModules: + """ + look in a given version of noredink-ui to get the versioned modules and the + latest version for each. + """ + + MODULE_RE = "(?PNri\.Ui\.\w+).V(?P\d+)" + + def __init__(self, elm_home, elm_version, package_name, package_version): + package_elm_json = os.path.join( + elm_home, elm_version, "package", package_name, package_version, "elm.json" + ) + with open(package_elm_json, "r") as fh: + self.module_list = json.load(fh)["exposed-modules"] + + self.versions = defaultdict(list) + for module in self.module_list: + match = re.match(self.MODULE_RE, module) + if match is None: + continue + + parts = match.groupdict() + self.versions[parts["name"]].append(int(parts["version"])) + self.versions[parts["name"]].sort() + + def latest_version(self, name): + if name in self.versions: + return max(self.versions[name]) + + def is_latest_version(self, name, version): + return self.latest_version(name) == version + + +class Import: + """a single import, with special handling for unversioned deprecated modules.""" + + DEPRECATED = "DEPRECATED" + + def __init__(self, filename, name, version): + self.filename = filename + self.name = name + + try: + self.version = int(version) + except ValueError: # "DEPRECATED" + self.version = version + + def to_dict(self): + return {"filename": self.filename, "name": self.name, "version": self.version} + + def __eq__(self, other): + return ( + self.filename == other.filename + and self.name == other.name + and self.version == other.version + ) + + def __hash__(self): + # objects aren't hashable by default but we need to use this one in a + # set. So we just make one big hash out of all the data we have! + return hash(self.filename + self.name + str(self.version)) + + def __str__(self): + return "{} imports {} version {}".format(self.filename, self.name, self.version) + + +class Imports(list): + """ + get all the imports in the project according to specific rules. + + this class is all about loading and storing this data, so we just subclass + `list` to save some hassle on writing iteration etc. + """ + + NRI_UI_IMPORT_RE = "import\s+" + NriUiModules.MODULE_RE + DEPRECATED_IMPORT_RE = "import\s+(?P[\w\.]*deprecated[\w\.]*)" + GENERIC_IMPORT_RE = "import\s+(?P[\w\.]+)" + + @classmethod + def from_source_directories(cls, source_directories, extras): + """ + construct a list of project imports given an elm.json's source directories. + """ + results = cls() + + for source_directory in source_directories: + for (dirpath, _, filenames) in os.walk(source_directory): + for filename in filenames: + if os.path.splitext(filename)[1] != ".elm": + continue + + full_path = os.path.join(dirpath, filename) + with open(full_path, "r") as fh: + lines = [line.strip() for line in fh.readlines()] + + for line in lines: + line = line.strip() + + # add whatever imports we want to track here, continue + # after each. + + match = re.match(cls.NRI_UI_IMPORT_RE, line) + if match is not None: + parts = match.groupdict() + + results.append( + Import( + filename=full_path, + name=parts["name"], + version=parts["version"], + ) + ) + continue + + match = re.match( + cls.DEPRECATED_IMPORT_RE, line, flags=re.IGNORECASE + ) + if match is not None: + parts = match.groupdict() + results.append( + Import( + filename=full_path, + name=parts["import"], + version=Import.DEPRECATED, + ) + ) + continue + + match = re.match(cls.GENERIC_IMPORT_RE, line) + if match is not None: + parts = match.groupdict() + if parts["import"] not in extras: + continue + + results.append( + Import( + filename=full_path, + name=parts["import"], + version=Import.DEPRECATED + ) + ) + continue + + return results + + @classmethod + def from_file(cls, filename): + try: + with open(filename, "r") as fh: + return cls([Import(**entry) for entry in csv.DictReader(fh)]) + except FileNotFoundError: + return cls([]) + + def to_file(self, filename, filter=None): + filter = filter or (lambda _: True) + + out = (entry.to_dict() for entry in self if filter(entry)) + + with open(filename, "w") as fh: + writer = csv.DictWriter(fh, ["filename", "name", "version"]) + writer.writeheader() + writer.writerows(out) + + +class Main: + def __init__(self, args): + self.args = args + self.elm_json = ElmJson(args.elm_json) + + self.deprecated = set(args.deprecate) + + # cache-y things. These get set away from `None` the first time their + # associated methods are called. This has the same effect as doing + # `@foo ||= bar` in Ruby. + self._current_imports = None + self._previous_imports = None + self._modules = None + + def run(self): + if self.args.command == "update": + return self.update() + elif self.args.command == "check": + return self.check() + elif self.args.command == "report": + return self.report() + else: + print("unrecognized command. Make sure the cases in main match the parser.") + return 1 + + def is_latest_version(self, name, version): + return self.modules().is_latest_version(name, version) and name not in self.deprecated + + def update(self): + self.current_imports().to_file( + self.args.imports_file, + filter=lambda import_: not self.is_latest_version(import_.name, import_.version), + ) + + return 0 + + def check(self): + new_imports = set(self.current_imports()) - set(self.previous_imports()) + new_deprecated_imports = set() + + for entry in new_imports: + if self.is_latest_version(entry.name, entry.version): + continue + + new_deprecated_imports.add(entry) + + status = 0 + + if new_deprecated_imports: + print("==== {} new deprecated imports".format(len(new_deprecated_imports))) + print( + "\n".join( + "{} (latest version: {})".format( + entry, + self.modules().latest_version(entry.name) + or "completely deprecated. Check noredink-ui?", + ) + for entry in new_deprecated_imports + ) + ) + print() + print( + textwrap.fill( + textwrap.dedent( + """ + If you meant to import a deprecated version, please run + `{}` and commit the changes to mark these deprecated + imports as acceptable. Otherwise, please upgrade to the + latest version (listed in each error above.) + """ + ), + width=80, + ).format(self.args.check_message_fix_command) + ) + status = 1 + + newly_removed = set(self.previous_imports()) - set(self.current_imports()) + if newly_removed: + print() + print("==== {} newly removed imports".format(len(newly_removed))) + print("\n".join(str(entry) for entry in newly_removed)) + print() + print( + "good job! Please run `{}` and commit the output to make sure these don't come back".format( + self.args.check_message_fix_command + ) + ) + status = 1 + + return status + + def report(self): + allowed_counts = {True: 0, False: 0} + counts = defaultdict(lambda: {True: 0, False: 0}) + + for entry in self.current_imports(): + allowed = self.is_latest_version(entry.name, entry.version) + + allowed_counts[allowed] += 1 + counts[entry.name][allowed] += 1 + + try: + widest = max(len(name) for name in counts) + except ValueError: # empty sequence + widest = 1 + + lines = [] + for name, alloweds in counts.items(): + total = alloweds[False] + alloweds[True] + percent = alloweds[True] / total + + out = "{} {: 4} of {: 4} ({:> 8.2%}) imports use the latest version".format( + name.ljust(widest), alloweds[True], total, percent + ) + + lines.append((percent, out)) + + lines.sort() + print("\n".join(line for (_, line) in lines)) + print() + + total = allowed_counts[False] + allowed_counts[True] + percent = allowed_counts[True] / (total or 1) + print( + "in total, {} of {} ({:> 8.2%}) imports use the latest version".format( + allowed_counts[True], total, percent + ) + ) + return 0 + + def previous_imports(self): + if self._previous_imports is None: + self._previous_imports = Imports.from_file(self.args.imports_file) + + return self._previous_imports + + def current_imports(self): + if self._current_imports is None: + self._current_imports = Imports.from_source_directories( + [ + os.path.join(os.path.dirname(self.args.elm_json), directory) + for directory in self.elm_json.source_directories + ], + self.deprecated + ) + + return self._current_imports + + def modules(self): + if self._modules is None: + try: + self._modules = NriUiModules( + self.args.elm_home, + self.elm_json.elm_version, + self.args.package, + self.elm_json.package_version(self.args.package), + ) + except FileNotFoundError: + print( + "elm.json for {} {} not found. Maybe run `elm make`?".format( + self.args.package, + self.elm_json.package_version(self.args.package), + ) + ) + sys.exit(1) + + return self._modules + + +def parser(): + out = argparse.ArgumentParser( + description=__doc__, formatter_class=argparse.ArgumentDefaultsHelpFormatter + ) + + out.add_argument( + "--check-message-fix-command", + help="update command to show in check error messages", + default=" ".join(sys.argv) + .replace("check", "update") + .replace("report", "update"), + ) + out.add_argument("--elm-json", help="which elm.json to use", default="ui/elm.json") + out.add_argument( + "--package", + help="which package to look for deprecations in", + default="NoRedInk/noredink-ui", + ) + out.add_argument( + "--elm-home", + help="where to look for package manifest", + default=os.environ.get("ELM_HOME", os.path.expanduser("~/.elm")), + ) + out.add_argument( + "--imports-file", + help="where do we store acceptable deprecated imports?", + default=os.path.join( + os.path.dirname(os.path.abspath(__file__)), + os.path.basename(__file__).replace(".py", ".import.csv"), + ), + ) + out.add_argument( + '--deprecate', + help="explicitly deprecate a module by name", + action="append", + default=[], + ) + + sub = out.add_subparsers() + sub.required = True + sub.dest = "command" + + sub.add_parser("update", help="update the acceptable import file") + sub.add_parser( + "check", help="check that we do not have any new instances of old modules" + ) + sub.add_parser("report", help="print an import report") + + return out + + +if __name__ == "__main__": + sys.exit(Main(parser().parse_args()).run()) From d790d4bc3ffcd631986f999ac60bbfdef83b2084 Mon Sep 17 00:00:00 2001 From: Brian Hicks Date: Wed, 4 Dec 2019 16:14:40 -0600 Subject: [PATCH 11/14] remove complexity we don't need in noredink-ui --- script/elm-deprecated-imports.py | 64 ++++++-------------------------- 1 file changed, 11 insertions(+), 53 deletions(-) diff --git a/script/elm-deprecated-imports.py b/script/elm-deprecated-imports.py index dbf29eef..100786f3 100755 --- a/script/elm-deprecated-imports.py +++ b/script/elm-deprecated-imports.py @@ -25,12 +25,7 @@ class ElmJson: self.data = json.load(fh) self.elm_version = self.data["elm-version"] - self.source_directories = self.data["source-directories"] - - def package_version(self, package): - return self.data["dependencies"]["direct"].get( - package, self.data["dependencies"]["indirect"].get(package, None) - ) + self.source_directories = ["src"] class NriUiModules: @@ -41,11 +36,8 @@ class NriUiModules: MODULE_RE = "(?PNri\.Ui\.\w+).V(?P\d+)" - def __init__(self, elm_home, elm_version, package_name, package_version): - package_elm_json = os.path.join( - elm_home, elm_version, "package", package_name, package_version, "elm.json" - ) - with open(package_elm_json, "r") as fh: + def __init__(self): + with open("elm.json", "r") as fh: self.module_list = json.load(fh)["exposed-modules"] self.versions = defaultdict(list) @@ -67,18 +59,11 @@ class NriUiModules: class Import: - """a single import, with special handling for unversioned deprecated modules.""" - - DEPRECATED = "DEPRECATED" - + """a single import.""" def __init__(self, filename, name, version): self.filename = filename self.name = name - - try: - self.version = int(version) - except ValueError: # "DEPRECATED" - self.version = version + self.version = int(version) def to_dict(self): return {"filename": self.filename, "name": self.name, "version": self.version} @@ -200,7 +185,7 @@ class Imports(list): class Main: def __init__(self, args): self.args = args - self.elm_json = ElmJson(args.elm_json) + self.elm_json = ElmJson("elm.json") self.deprecated = set(args.deprecate) @@ -252,7 +237,7 @@ class Main: "{} (latest version: {})".format( entry, self.modules().latest_version(entry.name) - or "completely deprecated. Check noredink-ui?", + or "completely deprecated.", ) for entry in new_deprecated_imports ) @@ -337,7 +322,7 @@ class Main: if self._current_imports is None: self._current_imports = Imports.from_source_directories( [ - os.path.join(os.path.dirname(self.args.elm_json), directory) + os.path.join(os.path.dirname("elm.json"), directory) for directory in self.elm_json.source_directories ], self.deprecated @@ -347,21 +332,7 @@ class Main: def modules(self): if self._modules is None: - try: - self._modules = NriUiModules( - self.args.elm_home, - self.elm_json.elm_version, - self.args.package, - self.elm_json.package_version(self.args.package), - ) - except FileNotFoundError: - print( - "elm.json for {} {} not found. Maybe run `elm make`?".format( - self.args.package, - self.elm_json.package_version(self.args.package), - ) - ) - sys.exit(1) + self._modules = NriUiModules() return self._modules @@ -374,27 +345,14 @@ def parser(): out.add_argument( "--check-message-fix-command", help="update command to show in check error messages", - default=" ".join(sys.argv) - .replace("check", "update") - .replace("report", "update"), - ) - out.add_argument("--elm-json", help="which elm.json to use", default="ui/elm.json") - out.add_argument( - "--package", - help="which package to look for deprecations in", - default="NoRedInk/noredink-ui", - ) - out.add_argument( - "--elm-home", - help="where to look for package manifest", - default=os.environ.get("ELM_HOME", os.path.expanduser("~/.elm")), + default=" ".join(sys.argv).replace("check", "update").replace("report", "update"), ) out.add_argument( "--imports-file", help="where do we store acceptable deprecated imports?", default=os.path.join( os.path.dirname(os.path.abspath(__file__)), - os.path.basename(__file__).replace(".py", ".import.csv"), + os.path.basename(__file__).replace(".py", ".csv"), ), ) out.add_argument( From 1e1182e43fbcee2097c7fb394d7d62993aa2038e Mon Sep 17 00:00:00 2001 From: Brian Hicks Date: Wed, 4 Dec 2019 16:14:50 -0600 Subject: [PATCH 12/14] create initial report of outdated module usage --- script/elm-deprecated-imports.csv | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) create mode 100644 script/elm-deprecated-imports.csv diff --git a/script/elm-deprecated-imports.csv b/script/elm-deprecated-imports.csv new file mode 100644 index 00000000..734eec0f --- /dev/null +++ b/script/elm-deprecated-imports.csv @@ -0,0 +1,30 @@ +filename,name,version +src/Nri/Ui/Page/V3.elm,Nri.Ui.Button,5 +src/Nri/Ui/Page/V3.elm,Nri.Ui.Text,2 +src/Nri/Ui/Page/V2.elm,Nri.Ui.Button,5 +src/Nri/Ui/Page/V2.elm,Nri.Ui.Text,2 +src/Nri/Ui/PremiumCheckbox/V3.elm,Nri.Ui.Checkbox,4 +src/Nri/Ui/PremiumCheckbox/V2.elm,Nri.Ui.Checkbox,3 +src/Nri/Ui/PremiumCheckbox/V1.elm,Nri.Ui.Checkbox,3 +src/Nri/Ui/SlideModal/V2.elm,Nri.Ui.Button,8 +src/Nri/Ui/SlideModal/V2.elm,Nri.Ui.Icon,3 +src/Nri/Ui/SlideModal/V2.elm,Nri.Ui.Text,2 +src/Nri/Ui/SlideModal/V1.elm,Nri.Ui.Button,8 +src/Nri/Ui/SlideModal/V1.elm,Nri.Ui.Icon,3 +src/Nri/Ui/SlideModal/V1.elm,Nri.Ui.Text,2 +src/Nri/Ui/Alert/V3.elm,Nri.Ui.Icon,3 +src/Nri/Ui/Alert/V2.elm,Nri.Ui.Icon,3 +src/Nri/Ui/Alert/V4.elm,Nri.Ui.Icon,3 +src/Nri/Ui/SortableTable/V1.elm,Nri.Ui.Table,4 +src/Nri/Ui/Button/V3.elm,Nri.Ui.Icon,3 +src/Nri/Ui/Button/V5.elm,Nri.Ui.Icon,3 +src/Nri/Ui/Button/V4.elm,Nri.Ui.Icon,3 +src/Nri/Ui/Button/V6.elm,Nri.Ui.Icon,4 +src/Nri/Ui/Button/V7.elm,Nri.Ui.Icon,4 +src/Nri/Ui/SegmentedControl/V6.elm,Nri.Ui.Icon,3 +src/Nri/Ui/Modal/V3.elm,Nri.Ui.Icon,3 +src/Nri/Ui/Modal/V5.elm,Nri.Ui.Modal,5 +src/Nri/Ui/Modal/V4.elm,Nri.Ui.Icon,3 +src/Nri/Ui/Modal/V6.elm,Nri.Ui.Modal,6 +src/Nri/Ui/Modal/V7.elm,Nri.Ui.Modal,7 +src/Nri/Ui/ClickableText/V1.elm,Nri.Ui.Icon,4 From b3e1a628c4e19a5305a5f0cc81400a7853729bb1 Mon Sep 17 00:00:00 2001 From: Brian Hicks Date: Wed, 4 Dec 2019 16:17:49 -0600 Subject: [PATCH 13/14] remove the elm- prefix --- script/{elm-deprecated-imports.csv => deprecated-imports.csv} | 0 script/{elm-deprecated-imports.py => deprecated-imports.py} | 0 2 files changed, 0 insertions(+), 0 deletions(-) rename script/{elm-deprecated-imports.csv => deprecated-imports.csv} (100%) rename script/{elm-deprecated-imports.py => deprecated-imports.py} (100%) diff --git a/script/elm-deprecated-imports.csv b/script/deprecated-imports.csv similarity index 100% rename from script/elm-deprecated-imports.csv rename to script/deprecated-imports.csv diff --git a/script/elm-deprecated-imports.py b/script/deprecated-imports.py similarity index 100% rename from script/elm-deprecated-imports.py rename to script/deprecated-imports.py From 9b9e1ea785a91f84b8d31c365ad3a66865a9e1cd Mon Sep 17 00:00:00 2001 From: Brian Hicks Date: Wed, 4 Dec 2019 16:22:29 -0600 Subject: [PATCH 14/14] add ratchet to CI --- .gitignore | 3 ++- Makefile | 9 +++++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/.gitignore b/.gitignore index 91ce31a0..bf093351 100644 --- a/.gitignore +++ b/.gitignore @@ -239,4 +239,5 @@ documentation.json .envrc /public -/tests/axe-report.json \ No newline at end of file +/tests/axe-report.json +/tests/deprecated-imports-report.txt diff --git a/Makefile b/Makefile index 47956435..a69c536e 100644 --- a/Makefile +++ b/Makefile @@ -6,6 +6,7 @@ test: node_modules npx elm-verify-examples --run-tests make axe-report make percy-tests + make deprecated-imports-report tests/axe-report.json: public script/run-axe.sh script/axe-puppeteer.js script/run-axe.sh > $@ @@ -18,6 +19,14 @@ axe-report: tests/axe-report.json script/format-axe-report.sh script/axe-report. percy-tests: script/percy-tests.sh +tests/deprecated-imports-report.txt: $(shell find src -type f) script/deprecated-imports.py + script/deprecated-imports.py report > $@ + +.PHONY: deprecated-imports-report +deprecated-imports-report: tests/deprecated-imports-report.txt script/deprecated-imports.py + @cat tests/deprecated-imports-report.txt + @script/deprecated-imports.py check + .PHONY: checks checks: script/check-exposed.py