Fix Tabs indicator style when changed via controlled mode #48

cyu06 · 2016-11-07T22:56:21Z

Fixes #32.
Fixes #47.

adidahiya · 2016-11-09T16:08:21Z

packages/core/src/components/tabs/tabs.tsx

+        if (newIndex !== prevState.selectedTabIndex) {
+            const tabElement = findDOMNode(this.refs[`tabs-${newIndex}`]) as HTMLElement;
+            // need to measure on the next frame in case the Tab children simultaneously change
+            setTimeout(() => { this.moveIndicator(tabElement); });


nit: I think you can omit the { } in this line

Thanks. Will update.

generally I would only address nits in CR if there are other necessary changes that need to happen in the same PR. don't want to cause friction with stuff like this

cyu06 · 2016-11-09T16:28:33Z

packages/core/src/components/tabs/tabs.tsx

@@ -110,7 +110,7 @@ export class Tabs extends AbstractComponent<ITabsProps, ITabsState> {
        if (newIndex !== prevState.selectedTabIndex) {
            const tabElement = findDOMNode(this.refs[`tabs-${newIndex}`]) as HTMLElement;
            // need to measure on the next frame in case the Tab children simultaneously change
-            setTimeout(() => { this.moveIndicator(tabElement); });
+            setTimeout(() => this.moveIndicator(tabElement));


Looking at line 105, I'm wondering if it is appropriate to also update the moveTimeout as well.

giladgray · 2016-11-09T19:01:15Z

packages/core/test/tabs/tabsTests.tsx

+                const style = wrapper.find(TabList).props().indicatorWrapperStyle;
+                assert.isDefined(style, "TabList should have a indicatorWrapperStyle prop set");
+                // "translateX(46px) translateY(0px)" -> 46
+                const actual = Number(style.transform.split(" ")[0].replace(/\D/g, ""));


might this be a little more legible if you tested whether the actual value contains ${expected}px?

instead of trying to parse the number out of the string (particularly brittle if we ever decide to change how the styles are applied), simply check whether 46px appears inside the style string.

giladgray · 2016-11-09T19:02:48Z

packages/core/test/tabs/tabsTests.tsx

+                const style = wrapper.find(TabList).props().indicatorWrapperStyle;
+                assert.isDefined(style, "TabList should have a indicatorWrapperStyle prop set");
+                // "translateX(46px) translateY(0px)" -> 46
+                const actual = Number(style.transform.split(" ")[0].replace(/\D/g, ""));


this code is identical to the test above. perhaps worth introducing a helper method that could do the entire check (plus my suggestion above)?

assertIndicatorPosition?

Agreed, thanks.

cyu06 · 2016-11-10T01:10:19Z

Not sure why my git fomo brought in all the commits, but it was just to check with master.

giladgray · 2016-11-10T02:28:39Z

something went wrong if it brought in all those commits... you might have to restart the branch and cherry-pick just the commits you care about.

giladgray · 2016-11-10T02:29:40Z

packages/core/test/tabs/tabsTests.tsx

    });

+    function assertIndicatorPosition(wrapper, TAB_INDEX_TO_SELECT) {


implicit any args. please type them. and name thusly: (wrapper, selectedTabIndex)

giladgray · 2016-11-10T02:33:08Z

packages/core/test/tabs/tabsTests.tsx

+            );
+            wrapper.setProps({ selectedTabIndex: TAB_INDEX_TO_SELECT });
+            // indicator moves via componentDidUpdate
+            setTimeout(() => {


in another PR you seemed to use wrapper.update() to get around this timeout. can we do that here?

https://github.com/palantir/blueprint/pull/41/files#diff-c5ca5cbfe301c1ea705d793f86ff1847R113

I can't do that here (it was before the moveIndicator logic moved). setProps already runs an update, but now that the indicator movement takes places in componentDidUpdate, so need to assert after the next frame.

blueprint-bot · 2016-11-10T15:14:34Z

Add types and code cleanup | Preview
_{core coverage | datetime coverage}

* Fix Tabs indicator style when changed via controlled mode * Move the moveIndicator to componentDidUpdate * Add types and code cleanup

Add design responsivity

Fix Tabs indicator style when changed via controlled mode

235c3d9

cyu06 added needs review Status: work in progress and removed needs review labels Nov 7, 2016

cyu06 force-pushed the cyu/fix-controlled-tabs-indicator branch from 6dbfc7a to 431a8a0 Compare November 8, 2016 21:09

Move the moveIndicator to componentDidUpdate

0ab43df

cyu06 force-pushed the cyu/fix-controlled-tabs-indicator branch from 431a8a0 to 0ab43df Compare November 8, 2016 21:22

cyu06 added needs review and removed Status: work in progress labels Nov 8, 2016

adidahiya added PR: waiting for reviewer and removed needs review labels Nov 9, 2016

adidahiya approved these changes Nov 9, 2016

View reviewed changes

Remove extraneous {}

415c330

cyu06 commented Nov 9, 2016

View reviewed changes

Update the moveTimeout

0684d6e

giladgray suggested changes Nov 9, 2016

View reviewed changes

cyu06 added Status: work in progress PR: waiting for reviewer and removed PR: waiting for reviewer Status: work in progress labels Nov 9, 2016

giladgray suggested changes Nov 10, 2016

View reviewed changes

Chris Yu added 2 commits November 10, 2016 06:51

Clean up test

0efde85

Add types and code cleanup

50fe50c

cyu06 force-pushed the cyu/fix-controlled-tabs-indicator branch from f8e46eb to 50fe50c Compare November 10, 2016 15:10

giladgray approved these changes Nov 11, 2016

View reviewed changes

giladgray merged commit 6cd5d8f into master Nov 11, 2016

giladgray deleted the cyu/fix-controlled-tabs-indicator branch November 11, 2016 18:24

leebyp mentioned this pull request Jan 20, 2017

Fix Tabs onChange handler called twice when controlled #520

Merged

1 task

peterblazejewicz pushed a commit to peterblazejewicz/blueprint that referenced this pull request Dec 20, 2017

Merge pull request palantir#48 from blueberryapps/add-responsivity

5430065

Add design responsivity

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Tabs indicator style when changed via controlled mode #48

Fix Tabs indicator style when changed via controlled mode #48

cyu06 commented Nov 7, 2016 •

edited

Loading

adidahiya Nov 9, 2016

cyu06 Nov 9, 2016

adidahiya Nov 9, 2016

cyu06 Nov 9, 2016

giladgray Nov 9, 2016

giladgray Nov 9, 2016

cyu06 Nov 9, 2016

cyu06 commented Nov 10, 2016

giladgray commented Nov 10, 2016

giladgray Nov 10, 2016

giladgray Nov 10, 2016

cyu06 Nov 10, 2016

blueprint-bot commented Nov 10, 2016

		});

		function assertIndicatorPosition(wrapper, TAB_INDEX_TO_SELECT) {

Fix Tabs indicator style when changed via controlled mode #48

Fix Tabs indicator style when changed via controlled mode #48

Conversation

cyu06 commented Nov 7, 2016 • 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

Choose a reason for hiding this comment

cyu06 commented Nov 10, 2016

giladgray commented Nov 10, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blueprint-bot commented Nov 10, 2016

cyu06 commented Nov 7, 2016 •

edited

Loading