feat(toggle): add new component and msft styles #212

nalogiudice · 2018-04-05T00:44:05Z

For details on formatting pull requests.

chrisdholt · 2018-04-05T00:48:32Z

packages/fast-components-class-name-contracts/src/index.ts

@@ -1,2 +1,3 @@
 export * from "./button";
+export * from "./toggle";


Likely should alphabetize

I think the components should be together, and then the "other" stuff should be separate (managed-classes in this case)

In theory, Foundation is a component.

This needs to be part of the linter if we're going to nit on it. Chris, if you're passionate about this being alphabetical please file an issue against fast-tslint-rules

Sure - didn't feel like a nit. But I'll file and take care of the issue. It does feel like something that's going to make it extremely hard for people to find things if we aren't grouping in a logical progression. Especially with a flat structure in src. I'll file an issue.

Synced with @nicholasrice on this. In the index.ts files where we import all things in "src" we will alphabetize. This does not need to be the case in .tsx files for regular imports. Unfortunately, due to some weird configs with .ts lint, we'll need to enforce this by principle. Trying to enforce alphabetizing in these files using .tslint would create a massive hinderance to our dev experience and overall code readability

chrisdholt · 2018-04-05T00:50:18Z

packages/fast-components-react-base/src/toggle/toggle.props.ts

+import * as React from "react";
+
+export interface IToggleProps {
+    children?: React.ReactNode | React.ReactNode[];


Is this really a place where we want to support anything?

In the label... sure why not? I don't think it needs to be text-only. If they want a glyph or hyperlink, they should be able to add one

I agree - requiring a string seems unnecessarily restrictive.

chrisdholt · 2018-04-05T00:50:45Z

packages/fast-components-react-base/src/toggle/toggle.tsx

+/**
+ * Toggle base component
+ */
+/* tslint:disable-next-line */


I think we can remove this tslint rule

ok - it's in the base component you put up so it should be removed from there too

chrisdholt · 2018-04-05T00:52:12Z

packages/fast-components-react-base/src/toggle/toggle.tsx

+ * Toggle base component
+ */
+/* tslint:disable-next-line */
+class Toggle extends Foundation<IToggleProps & IManagedClasses<IToggleClassNameContract>,  React.AllHTMLAttributes<HTMLElement>, IToggleState> {


We should try to be as specific as possible here. The reason that button supported All was due to the fact that there were serious conflicts when trying to allow HTMLAnchor | HTMLButton. Here though, I don't we lose the purpose of this if we tell the user they can pass an href.

chrisdholt · 2018-04-05T00:54:24Z

packages/fast-components-styles-msft/src/design-system/index.ts

@@ -10,4 +10,25 @@ const designSystemDefaults: IDesignSystem = {
    brandColor: "#0078D4"
 };

+/**
+ * Returns RGBA from hex value with optional alpha


we should pull this into a utilities folder if we're going to use this one. We may also want to just interpolate the values below

Also - questions as to whether or not this should live here or another package - colors, jss-utiltites...likely a team convo. But I'd pull from the design system folder.

We probably need to figure out the full color story or there is going to be a lot of redoing work. I could have simply added the rgba values, but this seems like something we would want anyway. I can move, but we should figure this out as a team.

Agree on syncing as a team here. Not saying it needs to land, but I think we can start categorizing in utils for now at the very least.

It would likely live in the color package - that already takes a dependency on chroma-js which probably has this functionality, so you might just use that

Cool, I'll look into that @nicholasrice

Ok @nicholasrice, I see a function think I can use from chroma-js to accomplish this. Do I need to add chroma to the styles-msft repo to use?

chrisdholt · 2018-04-05T00:56:06Z

packages/fast-components-styles-msft/src/toggle/index.ts

+    }
+};
+
+export default styles;


I think you need a newline here.

We need our linter or whatever to yell about this.

Yeah, it should be - not sure why it isn't

I believe I fixed an issue in the linter with a PR, if it's not updating let me know and I'll see if I can repro on your branch. There was an issue with Mac not jumping into all files.

Cool - will let you know.

chrisdholt · 2018-04-05T00:57:26Z

packages/fast-components-react-msft/src/toggle/index.ts

@@ -0,0 +1,6 @@
+import * as React from "react";
+import {  IFoundationProps, IToggleClassNameContract, IToggleProps, Toggle } from "@microsoft/fast-components-react-base";


can you remove the spacing between the brackets and the imports?

Yea the linter will fail this

UPDATE: Leave spacing here due to the linter not being able to lint this. It should now lint import and exports to include space. Within the ts files themselves, there should be no spaces.

chrisdholt · 2018-04-05T00:58:18Z

packages/fast-components-react-msft/src/toggle/examples.data.ts

+            unselectedString: "Off"
+        }
+    ]
+} as IGenericExample<IToggleProps>;


I think @nicholasrice added something that updates this to generate snapshots

+1 Check out the button component

yep all updated

chrisdholt · 2018-04-05T01:02:22Z

packages/fast-components-react-base/src/index.ts

@@ -2,6 +2,10 @@ import Button from "./button";
 export {Button};
 export * from "./button";

+import Toggle from "./toggle";


Should alphabetize

Like above, I think we should have our components separate from our other stuff. If we vote otherwise, I'm fine with it though.

Foundation is technically a component. I think maintainability is key unless we are going to comment and denote the intentional difference in structure. I think alphabetizing keeps everything in easy to understand and intuitive, but we can discuss as a team.

chrisdholt · 2018-04-05T01:02:52Z

packages/fast-components-react-base/app/examples.ts

@@ -1,2 +1,5 @@
 import ButtonExamples from "../src/button/examples.data";
 export {ButtonExamples};
+
+import ToggleExamples from "../src/toggle/examples.data";
+export {ToggleExamples};


Is this not yelling at you for a newline?

Nah, I can add it back in though. I received maybe one or two warnings about this at different times, but it seems all over the place when it "cares"

The rule is never more than one newline I believe, so this would be valid.

This would get caught on CI as well.

nalogiudice

Responded to some comments

nalogiudice · 2018-04-05T01:45:46Z

packages/fast-components-react-base/src/index.ts

@@ -2,6 +2,10 @@ import Button from "./button";
 export {Button};
 export * from "./button";

+import Toggle from "./toggle";


Like above, I think we should have our components separate from our other stuff. If we vote otherwise, I'm fine with it though.

nicholasrice · 2018-04-05T16:26:14Z

packages/fast-components-react-base/app/examples.ts

@@ -1,2 +1,5 @@
 import ButtonExamples from "../src/button/examples.data";
 export {ButtonExamples};
+
+import ToggleExamples from "../src/toggle/examples.data";
+export {ToggleExamples};


The rule is never more than one newline I believe, so this would be valid.

nicholasrice · 2018-04-05T16:58:23Z

packages/fast-components-react-base/src/toggle/toggle.props.ts

+import * as React from "react";
+
+export interface IToggleProps {
+    children?: React.ReactNode | React.ReactNode[];


I agree - requiring a string seems unnecessarily restrictive.

nicholasrice · 2018-04-05T16:59:19Z

packages/fast-components-react-base/src/toggle/toggle.props.ts

@@ -0,0 +1,13 @@
+import * as React from "react";
+
+export interface IToggleProps {


We've changed naming conventions for props slightly - can you align the prop names to the format button uses?

nicholasrice · 2018-04-05T17:06:18Z

packages/fast-components-react-base/src/toggle/toggle.tsx

+    }
+
+    /**
+     * Generates global aria-disabled


This comment doesn't really map to the function name - can you align the two?

or you could remove this method entirely - I think it makes sense when there is a bunch of conditional logic for rendering attributes, but if you're simply setting one attribute I think it is more readable to apply that in the render method directly.

<div aria-disabled={ this.props.disabled || null } //...

nicholasrice · 2018-04-05T17:11:24Z

packages/fast-components-react-base/src/toggle/toggle.tsx

+
+        return toggleAttributes;
+    }
+


same here - there doesn't seem to be much rhyme or reason why some attributes are applied here but others are applied in the render method- I'd just put all these in the render method and reduce some of the code-clutter that having an extra method creates.

nicholasrice · 2018-04-05T17:24:27Z

packages/fast-components-react-base/src/toggle/toggle.tsx

+                    />
+                    <span />
+                </div>
+                <span id={this.props.spanId}>


maybe we should call this statusLabel or something? spanId isn't a very semantic name.

Changed to statusLabelId to match other naming

nicholasrice · 2018-04-05T17:25:54Z

packages/fast-components-react-msft/src/toggle/examples.data.ts

+            unselectedString: "Off"
+        }
+    ]
+} as IGenericExample<IToggleProps>;


+1 Check out the button component

nicholasrice · 2018-04-05T17:26:36Z

packages/fast-components-react-msft/src/toggle/index.ts

@@ -0,0 +1,6 @@
+import * as React from "react";
+import {  IFoundationProps, IToggleClassNameContract, IToggleProps, Toggle } from "@microsoft/fast-components-react-base";


Yea the linter will fail this

nicholasrice · 2018-04-05T17:28:23Z

packages/fast-components-styles-msft/src/design-system/index.ts

@@ -10,4 +10,25 @@ const designSystemDefaults: IDesignSystem = {
    brandColor: "#0078D4"
 };

+/**
+ * Returns RGBA from hex value with optional alpha


It would likely live in the color package - that already takes a dependency on chroma-js which probably has this functionality, so you might just use that

nicholasrice · 2018-04-05T17:30:33Z

packages/fast-components-styles-msft/src/toggle/index.ts

+
+const styles: ComponentStyles<IToggleClassNameContract, IDesignSystem> = {
+    toggle: {
+        '& > label': {


+1

We're going to be mapping more directly to BEM principles - meaning that the span that indicates the status of the toggle would have a class directly associated to it, such as toggle_status_indicator

…dhere to FAST standards and BEM standards

nicholasrice · 2018-04-09T22:08:20Z

packages/fast-components-react-base/src/toggle/examples.data.ts

+    data: [
+        {
+            managedClasses: {
+                toggle: "toggle"            },


nit, formatting

nalogiudice requested review from chrisdholt and nicholasrice April 5, 2018 00:44

chrisdholt requested changes Apr 5, 2018

View reviewed changes

nalogiudice commented Apr 5, 2018

View reviewed changes

nicholasrice suggested changes Apr 5, 2018

View reviewed changes

awentzel changed the title ~~feat(toggle): adds toggle component and msft styles~~ feat(toggle): add new component and msft styles Apr 5, 2018

awentzel added feature A new feature type : feat and removed type : docs labels Apr 5, 2018

awentzel assigned nalogiudice Apr 5, 2018

nalogiudice force-pushed the users/nalogiudice/component-create-toggle branch 2 times, most recently from ddcc3dc to 03776d2 Compare April 11, 2018 09:09

nalogiudice added 9 commits April 11, 2018 11:14

Adds base structure for toggle - commiting to rebase

3503ecf

Finishes most styles, commiting to move to another branch

ea2e420

Finishes most styles, commiting to move to another branch

df15313

Finishes up last testing and additions

af173fa

feat(toggle): adds toggle component and msft styles

9ab1c4a

Updates branch after first round of reviews

e167edd

Updates color handling to use Chroma and re-architects JSS usage to a…

a41bc0f

…dhere to FAST standards and BEM standards

Removes onClick

2028b73

Updates pack.locks

d477813

nalogiudice force-pushed the users/nalogiudice/component-create-toggle branch from 5ffd72e to d477813 Compare April 11, 2018 18:14

nalogiudice added 4 commits April 11, 2018 12:33

Updates package.locks again

1a3bb74

Fixes bad rebase

92e06b1

Updates errors found via tslint

5a9cf2c

More linting updates

37b001c

nalogiudice force-pushed the users/nalogiudice/component-create-toggle branch from 8a815c8 to fc42419 Compare April 12, 2018 02:42

Adds chroma as a peerDependency

39dbed8

nalogiudice force-pushed the users/nalogiudice/component-create-toggle branch from fc42419 to 39dbed8 Compare April 12, 2018 04:00

Adds snapshots and fixes root package-lock json error

84135dc

nalogiudice force-pushed the users/nalogiudice/component-create-toggle branch from 37d507a to 84135dc Compare April 12, 2018 18:46

Adds props comments

a638d21

chrisdholt approved these changes Apr 12, 2018

View reviewed changes

nicholasrice approved these changes Apr 12, 2018

View reviewed changes

nalogiudice merged commit b9dd3e0 into master Apr 12, 2018

nalogiudice deleted the users/nalogiudice/component-create-toggle branch April 12, 2018 20:02

chrisdholt added pr : feature and removed pr : feature labels May 3, 2018

		@@ -1,2 +1,3 @@
		export * from "./button";
		export * from "./toggle";

		@@ -0,0 +1,6 @@
		import * as React from "react";
		import { IFoundationProps, IToggleClassNameContract, IToggleProps, Toggle } from "@microsoft/fast-components-react-base";

		@@ -0,0 +1,13 @@
		import * as React from "react";

		export interface IToggleProps {

feat(toggle): add new component and msft styles #212

feat(toggle): add new component and msft styles #212

Conversation

nalogiudice commented Apr 5, 2018

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

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

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

nalogiudice 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

nalogiudice Apr 9, 2018 • edited Loading

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

nalogiudice Apr 9, 2018 •

edited

Loading