[core:feature] Add Clickable component to wrap ClickableBehavior (wip) #387

kevinbarabash · 2019-04-19T14:50:06Z

I've wanted to create a Clickable component for some time now to make ClickableBehavior easier to use. This needs comments, better docs, and tests, but I thought I put it up early so that people could see what I was thinking with this wrapper. I'd like to eventually update the existing component that use ClickableBehavior to use Clickable instead.

jeresig · 2019-04-19T14:51:58Z

Deploy preview for wonder-blocks ready!

Built with commit 41a5811

https://deploy-preview-387--wonder-blocks.netlify.com

codecov · 2019-04-19T14:52:01Z

Codecov Report

Merging #387 into master will increase coverage by 0.04%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #387      +/-   ##
==========================================
+ Coverage   94.81%   94.85%   +0.04%     
==========================================
  Files         111      112       +1     
  Lines        1619     1633      +14     
  Branches      321      325       +4     
==========================================
+ Hits         1535     1549      +14     
  Misses         79       79              
  Partials        5        5

Impacted Files	Coverage Δ
...onder-blocks-core/components/clickable-behavior.js	`97.56% <ø> (ø)`	⬆️
...es/wonder-blocks-clickable/components/clickable.js	`100% <100%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9f9d5d7...41a5811. Read the comment docs.

jeresig

Very cool! It seems like this is going to be quite useful. Pushing back as I think we'd really benefit from some more-detailed documentation, especially making it clear when we expect people to be using this (or not).

jeresig · 2019-04-19T17:15:21Z

packages/wonder-blocks-core/components/clickable.js

@@ -0,0 +1,141 @@
+// @flow
+import * as React from "react";
+import {StyleSheet} from "aphrodite";


Before we add this to core - we should ask if we want this in core (since otherwise will require a version change later to move it). I guess if we ever wanted to move getClickableBehavior out of core and this into a "clickable" package we'd have to bump the version anyway.

Good call. Maybe we could create a wonder-blocks-clickable package. We can move ClickableBehavior over that package but still have wonder-blocks-core export ClickableBehavior though I think we'd want to eventually drop that export and have webapp just use Clickable and have ClickableBehavior be completely private.

jeresig · 2019-04-19T17:16:29Z

packages/wonder-blocks-core/components/clickable.js

+const StyledButton = addStyle<"button">("button");
+const StyledLink = addStyle<typeof Link>(Link);
+
+export default class Clickable extends React.Component<Props> {


Can you add a detailed comment here explaining how this component should be used? I think you can move the comment that you currently have at the top of clickable.md into here and maybe expand it a bit explaining the usage and especially explaining when you should be using it and not Link or Button.

jeresig · 2019-04-19T17:19:44Z

packages/wonder-blocks-core/components/clickable.js

+import type {StyleType} from "../util/types.js";
+
+type Props = {|
+    "aria-label": string,


Can you add comments to these props so that they show up in the documentation? You can probably copy most of them from Link or Button.

somewhatabstract · 2019-04-19T17:43:27Z

packages/wonder-blocks-core/components/clickable.js

+                            >
+                                {content}
+                            </StyledButton>
+                        );


Tempted to tidy this up by having some separate methods like maybeRenderStyledButton, etc. and then have them control the conditions of if they render. Or at least have renderStyledButton and have it extract the props it needs. I think it would make the code a little easier to follow.

jandrade · 2019-04-19T21:25:10Z

packages/wonder-blocks-core/components/clickable.js

+import * as React from "react";
+import {StyleSheet} from "aphrodite";
+
+import {Link} from "react-router-dom";


maybe arriving a bit late to the party.... just wondering, is there a strong reason for including react-router-dom as a WB dependency? instead of letting webapp handling directly the routing? Asking these questions b/c I would like to understand the context 😃

I wish we didn't, but we wanted to be able to use client-side routing with wonder-blocks components and being able to set href="/foo/bar" on components seemed a convenient way to specify routes. I couldn't figure how to make this work without importing Link from react-router-dom within wonder-blocks. 😅

jandrade · 2019-04-19T21:27:26Z

packages/wonder-blocks-core/components/clickable.md

+<View>
+    <Clickable>
+        {(state) => {
+            console.log(state);


maybe this line could be removed?

…o clickable

kevinbarabash

I can't formally "request changes" on a PR that I'm the author. I've left some comments that should be addressed. In particular deduplicating the types.js, removing the old files from wonder-blocks-core, and adding comments for the props in clickable.js stand out.

packages/wonder-blocks-clickable/components/clickable.md

kevinbarabash · 2019-07-01T23:30:42Z

packages/wonder-blocks-clickable/components/clickable.js

+                                {...commonProps}
+                                to={href}
+                                onClick={onClick}
+                                aria-disabled={disabled ? "true" : undefined}


This looks better than what we had before. Nice!

kevinbarabash · 2019-07-01T23:32:12Z

packages/wonder-blocks-clickable/components/clickable.js

+}
+
+const styles = StyleSheet.create({
+    reset: {


Wowza! If you grabbed this reset from somewhere please a link a comment.

this was a part of the old Clickable component i took over

Hmm... I wonder where I found it from. 🤔 😅

It looks like it was based on https://gist.github.com/MoOx/9137295. Can you add a link to that file in a comment?

packages/wonder-blocks-clickable/components/clickable.js

packages/wonder-blocks-clickable/components/clickable.test.js

packages/wonder-blocks-clickable/package.json

kevinbarabash · 2019-07-01T23:47:59Z

packages/wonder-blocks-clickable/package.json

@@ -0,0 +1,39 @@
+{
+    "name": "@khanacademy/wonder-blocks-clickable",
+    "version": "1.0.0",


With new packages I'd usually say start this at 0.1.0 and set private: true, but given this is a very small package, I think we can get it done in a single PR so this should be okay.

I'd err on the side of staying private: true and version 0.1.0. It's easier to go from this to public release than to discover a problem at public release and have to back pedal. I prefer to plan for the worst rather than hope for the best. What do you think?

+1 for private: true and adding 0.1.0. By adding this initial version, lerna will try to publish 1.0.0 instead of 1.0.1 (or something similar). Nothing that impacts at all, but good for keeping semver consistency.

packages/wonder-blocks-clickable/components/clickable.js

kevinbarabash · 2019-07-01T23:59:14Z

packages/wonder-blocks-core/index.js

@@ -2,6 +2,7 @@
 import type {
    ClickableHandlers,
    ClickableState,
+    ClickableRole,


Now that wonder-blocks-clickable is exporting this we can remove this from wonder-blocks-core.

somewhatabstract · 2019-07-02T16:07:41Z

packages/wonder-blocks-clickable/components/clickable.js

+                    const activeHref = href && !disabled;
+                    const useClientRouter = router && !skipClientNav;
+
+                    if (activeHref && useClientRouter) {


What do you think about putting the main bulk of this inline function into a separate method or set of methods? I think it might make the code a little easier to read by reducing indentation?

I agree, I think that’d be better

somewhatabstract · 2019-07-02T16:07:56Z

packages/wonder-blocks-clickable/components/clickable.js

+                                {content}
+                            </StyledLink>
+                        );
+                    } else if (activeHref && !useClientRouter) {


This can just be an if since the previous condition returns.

I feel like it’s better this way for clarity, what do you think?

somewhatabstract · 2019-07-02T16:08:11Z

packages/wonder-blocks-clickable/components/clickable.js

+                                {content}
+                            </StyledAnchor>
+                        );
+                    } else {


This else is unnecessary since both previous conditions return anyway.

same as above

somewhatabstract · 2019-07-02T16:09:11Z

packages/wonder-blocks-clickable/components/clickable.md

+        alignItems: "center",
+    },
+    h1: {
+        marginRight: "25px",


What do you think about using Wonder Blocks Spacing constants in our docs? Seems like this is an opportunity to encourage best practices.

I can’t get spacing to work, when I apply it here it has no effect :/

Have a look at the uses of wonder-blocks-spacing in the other markdown files.

somewhatabstract

Looks good. I have a few questions inline for your consideration.

…lickable

…o clickable

kevinbarabash

Some minor nits in clickable.md.

kevinbarabash · 2019-07-02T20:25:24Z

packages/wonder-blocks-clickable/components/clickable.md

+
+const styles = StyleSheet.create({
+    focused: {
+        border: "none",


It's hard to see this style since the unfocused state has border: "none" as well.

i’ll just remove it

kevinbarabash · 2019-07-02T20:29:41Z

packages/wonder-blocks-clickable/components/clickable.md

+    <Clickable onClick={() => alert("You clicked some text!")}>
+        {
+            eventState =>
+                <Body style={[


Can you wrap this in a View to make it look more like a card and change the background color of it as the eventState changes?

kevinbarabash

Thanks for the changes. The only blocking issue now is that handlers not being passed to getCorrectTag.

kevinbarabash · 2019-07-08T14:25:05Z

packages/wonder-blocks-clickable/components/clickable.js

-            <ClickableBehavior>
-                {(state, handlers) =>
+            <ClickableBehavior onClick={this.props.onClick}>
+                {(state) =>


Not passing the handlers means that the state won't get update correctly.

kevinbarabash · 2019-07-08T14:25:50Z

packages/wonder-blocks-clickable/components/clickable.js

@@ -18,12 +18,12 @@ type Props = {|
    children: (eventState: ClickableState) => React.Node,

    /**
-     * An onClick function which Clickable can execure
+     * An onClick function which Clickable can execute when clicked


Thanks for fixing my typos.

kevinbarabash

LGTM. @jandrade will have to approve this PR though since I'm still its author.

…lickable

jandrade

Everything looks good so far. Just added some comments that need to be addressed.

jandrade · 2019-07-09T01:52:17Z

packages/wonder-blocks-clickable/package.json

@@ -0,0 +1,39 @@
+{
+    "name": "@khanacademy/wonder-blocks-clickable",
+    "version": "1.0.0",


+1 for private: true and adding 0.1.0. By adding this initial version, lerna will try to publish 1.0.0 instead of 1.0.1 (or something similar). Nothing that impacts at all, but good for keeping semver consistency.

jandrade · 2019-07-09T01:55:51Z

packages/wonder-blocks-clickable/util/types.js

+// Source: https://www.w3.org/WAI/PF/aria-1.1/states_and_properties
+// TODO(kevinb): convert to disjoint union of exact object types keyed on role
+// eslint-disable-next-line flowtype/require-exact-type
+export type AriaProps = {|


e.g. If you needed it, you shouldn't need to export AriaProps here. Instead, you could import directly from wonderblocks-core.

it actually wasn’t being used at all, I removed it

packages/wonder-blocks-clickable/components/clickable.js

jandrade · 2019-07-09T02:08:29Z

packages/wonder-blocks-clickable/components/clickable.js

+                        // eslint-disable-next-line react/prop-types
+                        "aria-label": this.props["aria-label"],
+                        "data-test-id": this.props.testId,
+                        role: this.props.role,


This role prop shouldn't be part of commonProps b/c <button> doesn't need to use this role.

More info here: https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/button_role#Best_practices

thanks for catching that!

jandrade

LGTM! Great job implementing this component :). Thanks for addressing my comments.

somewhatabstract · 2019-07-09T20:09:20Z

packages/wonder-blocks-clickable/components/clickable.test.js

+    });
+
+    test("disallow navigation when href and disabled are both set", () => {
+        const wrapper = mount(


NIT: Missing // Arrange

good catch :)

somewhatabstract

LGTM

jeresig

Excellent, thank you!

[core:feature] Add Clickable component to wrap ClickableBehavior (wip)

c48efde

kevinbarabash requested review from jeresig, jandrade, jsatk and somewhatabstract April 19, 2019 14:50

jeresig requested changes Apr 19, 2019

View reviewed changes

somewhatabstract reviewed Apr 19, 2019

View reviewed changes

jandrade reviewed Apr 19, 2019

View reviewed changes

kevinbarabash mentioned this pull request May 11, 2019

Add wallaby.js config, update enzyme, add more tests for ActionMenu and ActionItem #395

Merged

kevinbarabash assigned kumailn Jul 1, 2019

Kumail Naqvi added 5 commits July 1, 2019 15:45

Create Clickable compoenent package

9db4f01

update clickable examples

8650739

update clickable examples

9a3d169

Merge branch 'clickable' of https://github.com/Khan/wonder-blocks int…

2ae5466

…o clickable

update comments

026c92f

kumailn requested review from jandrade and somewhatabstract July 1, 2019 23:39

kevinbarabash commented Jul 2, 2019

View reviewed changes

update package.json, tests, examples

141301f

somewhatabstract reviewed Jul 2, 2019

View reviewed changes

somewhatabstract requested changes Jul 2, 2019

View reviewed changes

Kumail Naqvi added 2 commits July 2, 2019 10:19

update snapshot

1cbe5bc

refactor Clickable implementation, update example & snpashot

560109a

kumailn requested a review from somewhatabstract July 2, 2019 18:57

kumailn requested a review from jeresig July 2, 2019 18:58

Kumail Naqvi added 4 commits July 2, 2019 11:58

Merge branch 'master' into clickable

69f3280

update snapshot

d90e9ed

Merge branch 'master' of https://github.com/Khan/wonder-blocks into c…

654ad34

…lickable

Merge branch 'clickable' of https://github.com/Khan/wonder-blocks int…

d13782e

…o clickable

kevinbarabash commented Jul 2, 2019

View reviewed changes

Kumail Naqvi added 2 commits July 3, 2019 09:54

update examples

c20da68

update examples, refactor, update snapshots

dc92e0a

kevinbarabash commented Jul 8, 2019

View reviewed changes

pass handlers

f891ce9

kevinbarabash commented Jul 8, 2019

View reviewed changes

Merge branch 'master' of https://github.com/Khan/wonder-blocks into c…

e3680d1

…lickable

jandrade requested changes Jul 9, 2019

View reviewed changes

jandrade reviewed Jul 9, 2019

View reviewed changes

Kumail Naqvi added 2 commits July 9, 2019 09:40

update package, remove utils

c9d7eb7

add comment, update role

bf3afed

jandrade approved these changes Jul 9, 2019

View reviewed changes

somewhatabstract reviewed Jul 9, 2019

View reviewed changes

somewhatabstract approved these changes Jul 9, 2019

View reviewed changes

add arrange comment

41a5811

kumailn requested a review from somewhatabstract July 9, 2019 20:26

somewhatabstract approved these changes Jul 9, 2019

View reviewed changes

jeresig approved these changes Jul 9, 2019

View reviewed changes

kumailn merged commit 4cadfc8 into master Jul 9, 2019

somewhatabstract deleted the clickable branch October 2, 2019 17:55

[core:feature] Add Clickable component to wrap ClickableBehavior (wip) #387

[core:feature] Add Clickable component to wrap ClickableBehavior (wip) #387

Conversation

kevinbarabash commented Apr 19, 2019

jeresig commented Apr 19, 2019 • edited

codecov bot commented Apr 19, 2019 • edited

Codecov Report

jeresig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

somewhatabstract Apr 19, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinbarabash left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kumailn Jul 2, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

somewhatabstract left a comment

Choose a reason for hiding this comment

kevinbarabash left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinbarabash left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinbarabash left a comment

Choose a reason for hiding this comment

jandrade left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jandrade left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

somewhatabstract left a comment

Choose a reason for hiding this comment

jeresig left a comment

Choose a reason for hiding this comment

jeresig commented Apr 19, 2019 •

edited

codecov bot commented Apr 19, 2019 •

edited

somewhatabstract Apr 19, 2019 •

edited

kumailn Jul 2, 2019 •

edited