className prop consistency #1950

kmblake · 2017-12-20T21:58:25Z

Checklist

Enable CircleCI for your fork
Include tests
Update documentation

Changes proposed in this pull request:

fix className support for the following components:

Hotkey
Hotkeys
KeyCombo
QueryList (select)
ResizeHandle (table)

palantirtech · 2017-12-20T21:58:26Z

Thanks for your interest in palantir/blueprint, @kmblake! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request.

giladgray · 2017-12-20T22:03:15Z

packages/core/src/components/hotkeys/hotkey.tsx

@@ -61,6 +62,12 @@ export interface IHotkeyProps {
     */
    stopPropagation?: boolean;

+    /**
+     * Space-delimited string of class names applied to the
+     * keyCombo contained in the hotkey.


backticks around "KeyCombo" (also capital K)

giladgray · 2017-12-20T22:03:58Z

packages/core/src/components/hotkeys/hotkeys.tsx

@@ -52,6 +53,7 @@ export class Hotkeys extends AbstractPureComponent<IHotkeysProps, {}> {

        let lastGroup = null as string;
        const elems = [] as JSX.Element[];
+        const rootClasses = classNames("pt-hotkey-column", this.props.className);


move this assignment after the for loop, right before the return -- this location separates the loop vars from their usage.

giladgray · 2017-12-20T22:05:19Z

packages/test-commons/src/generateIsomorphicTests.ts

 */
 export function generateIsomorphicTests(
    Components: { [name: string]: any },
    props: { [name: string]: any },
    children: { [name: string]: React.ReactNode },
-    skipList: string[] = [],
+    renderSkipList: string[] = [],
+    classNameSkipList: string[] = ["Alert", "Dialog", "MenuItem", "Popover2", "Portal", "Toaster", "Tooltip2"],


remove this default value, use []. set this array in packages/core/test/isotest.js

Ah that makes sense! Moved all customizations to packages/core/test/isotest.js

giladgray · 2017-12-20T22:05:38Z

packages/test-commons/src/generateIsomorphicTests.ts

@@ -55,6 +57,24 @@ export function generateIsomorphicTests(
                        Enzyme.render(element);
                    });
                }
+                if (classNameSkipList.includes(componentName)) {
+                    it.skip(`<${componentName}>`);


use the same test name as line 63.

I refactored this so that if a component is on the skipList, both the render and className tests will be skipped. If rendering fails, no use searching for a class

giladgray · 2017-12-20T22:05:46Z

packages/test-commons/src/generateIsomorphicTests.ts

+                if (classNameSkipList.includes(componentName)) {
+                    it.skip(`<${componentName}>`);
+                } else {
+                    it(`<${componentName}>  className is applied to outer element`, () => {


extra space after >

giladgray · 2017-12-20T22:07:04Z

packages/test-commons/src/generateIsomorphicTests.ts

@@ -55,6 +57,24 @@ export function generateIsomorphicTests(
                        Enzyme.render(element);
                    });
                }
+                if (classNameSkipList.includes(componentName)) {


🤔 it's not so much about skipping as about elements where className is not on the root -- but they should still support className.

maybe we can always have a unit test but if the component is in the list then we use Enzyme.render(element).find(".test")?

Retitled to classNameChildList and made it so for anything on that list the test will search the whole component tree for the class.

giladgray · 2017-12-20T22:07:48Z

packages/test-commons/src/generateIsomorphicTests.ts

+                                className: "test",
+                                iconName: "pt-icon-build",
+                                inline: true,
+                                lazy: false,


why are these three props necessary? should only have to set className, other required props should come from props arg itself.

Moved these to the isotest.js files so they will only be used for the components that need them

blueprint-bot · 2017-12-21T01:36:58Z

Cleaned up isotests

Preview: documentation | table

kmblake · 2017-12-21T01:44:32Z

packages/table/test/isotest.js

+    "CopyCellsMenuItem"
+]
+
+const skipList = [


@giladgray Classname doesn't propagate for these three components, but i'm not sure of the best way to handle these. It looks like they are just wrappers for eventHandlers, in which case maybe className doesn't need to propagate?

ResizeHandle - add support

the other two, you're right, they're the exceptions!

blueprint-bot · 2017-12-21T02:11:05Z

Made className prop for resizeHandle

Preview: documentation | table

giladgray

looks great @kmblake!!

giladgray · 2017-12-21T18:39:05Z

packages/test-commons/src/generateIsomorphicTests.ts

+                            {
+                                ...props[componentName],
+                                className: "test",
+                            } as any,


is this as any necessary? I imagine it shouldn't be.

cmslewis

One nit but nothing blocking.

cmslewis · 2017-12-21T19:20:13Z

packages/test-commons/src/generateIsomorphicTests.ts

@@ -38,6 +39,7 @@ export function generateIsomorphicTests(
    props: { [name: string]: any },
    children: { [name: string]: React.ReactNode },
    skipList: string[] = [],
+    classNameChildList: string[] = [],


Need to document this new param in the function comment above.

cmslewis · 2017-12-21T19:22:35Z

packages/core/test/isotest.js

+    "Alert",
+    "Dialog",
+    "MenuItem",
+    "Popover2",


Will need to return and remove the 2s here after #1946 and #1370 merge.

blueprint-bot · 2018-01-08T18:56:42Z

Documenting new param

Preview: documentation | table

kmblake · 2018-01-08T19:44:10Z

Not sure why brings focus to overlay if autoFocus=true suddenly started failing...it's passing when I run the test locally

cmslewis · 2018-01-08T20:54:45Z

@kmblake it's just a flaky test. Try re-rerunning from the Circle UI.

giladgray

@kmblake this is almost there! just two little things and i think we're good to go. let me know if you're too busy, I'm happy to jump in.

giladgray · 2018-01-08T23:56:26Z

packages/test-commons/src/generateIsomorphicTests.ts

+                            children[componentName],
+                        );
+                        if (classNameChildList.includes(componentName)) {
+                            assert.isAtLeast(Enzyme.shallow(element).find(".test").length, 1);


❌ shallow and render are very different... we want to use render only here as it produces a DOM tree, whereas shallow produces some Enzyme-only tree that doesn't actually assert server-side-rendering success. should be possible to simply replace the word shallow with render, it still has a find method.

giladgray · 2018-01-08T23:57:12Z

packages/test-commons/src/generateIsomorphicTests.ts

@@ -32,12 +33,15 @@ function isReactClass(Component: any): boolean {
 * @param props custom props per component
 * @param children custom children per component
 * @param skipList array of component names to skip
+ * @param classNameChildList array of component names for which className is legitimately passed down to
+ * child element


array of component names where className is rendered on an arbitrary child element, rather than directly on the root element.

blueprint-bot · 2018-01-09T00:07:35Z

Replaced shallow with render in className test

Preview: documentation | table

giladgray · 2018-01-09T00:13:30Z

packages/test-commons/src/generateIsomorphicTests.ts

+                            children[componentName],
+                        );
+                        if (classNameChildList.includes(componentName)) {
+                            assert.isAtLeast(Enzyme.render(element).find(".test").length, 1);


looks like this change caused some tests to fail, but this is definitely more correct.

could add { inline: true } prop to those components to keep everything inside the container element.

It seems like this gets a little tricky for a few components. Alert and Dialog render className on a child div within an Overlay element, but that div isn't rendered unless isOpen: true is passed as a prop. But if isOpen: true is passed in, then we get an error about "document not defined." Is there an easy way to mock the document for these components to attach to?

aw nuts. there is certainly not an easy way to mock the document. i can take a look at this.

giladgray · 2018-01-09T00:14:03Z

packages/test-commons/src/generateIsomorphicTests.ts

@@ -54,6 +58,21 @@ export function generateIsomorphicTests(
                        // errors will fail the test and log full stack traces to the console. nifty!
                        Enzyme.render(element);
                    });
+                    it(`<${componentName}> className is applied to outer element`, () => {


this test name is not always accurate... might want to do the classNameChildList check outside this it() so we can change the test name?

- use Enzyme.shallow + disable lifecycle methods to get JSX tree - don't care where className appears in tree, just that it does

giladgray · 2018-01-11T21:50:04Z

@kmblake I just pushed a commit, attempting to fix the build. i've refactored the className test to use Enzyme.shallow and disabling lifecycle methods to find className anywhere in the JSX tree without overcomplicating the render. nicely, this allowed me to remove the 5th argument you added to the isotest function as all components look the same in this manner: Portal is just a node like any other.

assuming the build will fail again on a few components--i've got local fixes but want to know which ones are actually necessary.

fix className in QueryList renderer (per #1993)

blueprint-bot · 2018-01-11T21:55:27Z

refactor className isotest:

Preview: documentation | landing | table

giladgray · 2018-01-11T22:03:49Z

oops i fucked up, these are all false positives. and now i can't make it actually fail correctly.

giladgray · 2018-01-11T22:08:15Z

ok I just removed the new isotest entirely -- writing this test in a generic way is just too finicky.

blueprint-bot · 2018-01-11T22:14:37Z

remove className isotest entirely -- false positives, just no good.

Preview: documentation | landing | table

addressed

adidahiya · 2018-01-12T20:55:20Z

packages/core/src/components/hotkeys/hotkey.tsx

+     * Space-delimited string of class names applied to the
+     * `KeyCombo` contained in the hotkey.
+     */
+    keyComboClassName?: string;


why do we need this?

we don't strictly need it and it's probably safe to remove. shall i do that?

adidahiya · 2018-01-12T20:55:58Z

packages/core/test/isotest.js

@@ -3,6 +3,7 @@
 * Licensed under the terms of the LICENSE file distributed with this project.
 */

+// @ts-check


adidahiya · 2018-01-12T20:56:58Z

packages/test-commons/src/generateIsomorphicTests.ts

@@ -32,6 +32,7 @@ function isReactClass(Component: any): boolean {
 * @param props custom props per component
 * @param children custom children per component
 * @param skipList array of component names to skip
+ * element, rather than directly on the root element


stray comment? what's this?

…Name-consistency

blueprint-bot · 2018-01-25T01:24:41Z

remove stray comment and keyComboClassName

Preview: documentation | landing | table

kmblake added 3 commits December 19, 2017 17:41

Made className propogate for hotkey components

35ce05b

Adding className consistency test

33baa2e

Merge branch 'master' into kb/className-consistency

d93c4f2

giladgray changed the title ~~Kb/class name consistency~~ className prop consistency Dec 20, 2017

giladgray suggested changes Dec 20, 2017

View reviewed changes

kmblake added 2 commits December 20, 2017 15:03

Hotkey styling fixes

f11d32c

Cleaned up isotests

1230c05

kmblake commented Dec 21, 2017

View reviewed changes

Made className prop for resizeHandle

d16d86b

giladgray approved these changes Dec 21, 2017

View reviewed changes

cmslewis previously approved these changes Dec 21, 2017

View reviewed changes

Documenting new param

853b14e

giladgray previously requested changes Jan 8, 2018

View reviewed changes

Replaced shallow with render in className test

919559e

giladgray reviewed Jan 9, 2018

View reviewed changes

refactor className isotest:

7d97b3b

- use Enzyme.shallow + disable lifecycle methods to get JSX tree - don't care where className appears in tree, just that it does

giladgray mentioned this pull request Jan 11, 2018

Use className prop in Select and Suggest #1993

Merged

Gilad Gray added 2 commits January 11, 2018 13:52

missed an isotest file

6a57ebf

fix className in QueryList renderer

e1ecfcb

fix className in QueryList renderer (per #1993)

remove className isotest entirely -- false positives, just no good.

8666ad9

adidahiya added this to the 2.0.0-beta.1 milestone Jan 12, 2018

adidahiya suggested changes Jan 12, 2018

View reviewed changes

adidahiya modified the milestones: 2.0.0-beta.1, 2.0.0 Jan 12, 2018

adidahiya changed the base branch from master to develop January 15, 2018 17:12

Gilad Gray added 2 commits January 24, 2018 17:19

Merge branch 'develop' of github.com:palantir/blueprint into kb/class…

3caa3af

…Name-consistency

remove stray comment and keyComboClassName

324372f

adidahiya approved these changes Jan 25, 2018

View reviewed changes

giladgray merged commit f4204cc into develop Jan 25, 2018

giladgray deleted the kb/className-consistency branch January 25, 2018 19:13

className prop consistency #1950

className prop consistency #1950

Conversation

kmblake commented Dec 20, 2017 • edited by adidahiya Loading

Checklist

Changes proposed in this pull request:

palantirtech commented Dec 20, 2017

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

blueprint-bot commented Dec 21, 2017

Cleaned up isotests

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blueprint-bot commented Dec 21, 2017

Made className prop for resizeHandle

giladgray left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmslewis 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

blueprint-bot commented Jan 8, 2018

Documenting new param

kmblake commented Jan 8, 2018

cmslewis commented Jan 8, 2018

giladgray left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blueprint-bot commented Jan 9, 2018

Replaced shallow with render in className test

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

giladgray commented Jan 11, 2018

blueprint-bot commented Jan 11, 2018

refactor className isotest:

giladgray commented Jan 11, 2018 • edited Loading

giladgray commented Jan 11, 2018 • edited Loading

blueprint-bot commented Jan 11, 2018

remove className isotest entirely -- false positives, just no good.

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

blueprint-bot commented Jan 25, 2018

remove stray comment and keyComboClassName

kmblake commented Dec 20, 2017 •

edited by adidahiya

Loading

giladgray commented Jan 11, 2018 •

edited

Loading

giladgray commented Jan 11, 2018 •

edited

Loading