re-add segment metadata fix #1086 fix #1047 #1088

mattkrick · 2017-06-17T07:27:07Z

test:

check the sent packets & make sure the call to segment is getting there with all the data (namely params)
make sure title is correct after a refresh (ie hit refresh on /me)
make sure title is correct after updating a component (ie click between teams)

First, this solves #1047 by reusing the same function instead of creating a new function inside the render cycle.

Next, this replaces the internal extra with extraProps. with extra, we assumed that the props that react-router gave our async route would not change throughout versions. that's not right. now, react-router can continue to add fields & we don't need to worry about them since our props are passed through explicitly.

Then, we add a bottom param to AsyncRoute that is a flag to signify that there are no more abstract routes below that one.

finally, this creates a previousLocation singleton that is passed in as context in ActionContainer. This is necessary because segment wants to know the referrer AND the params. Params are not established until it hits the bottom of the routing tree. sending them back to the top a la redux would not work because that would cause a new render cycle, which means lastPage == nextPage. So, sending down the lastPage (now called lastPath since it is a pathname) is necessary.

codecov · 2017-06-17T07:27:10Z

Codecov Report

Merging #1088 into master will increase coverage by 11.86%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #1088       +/-   ##
==========================================
+ Coverage   45.13%     57%   +11.86%     
==========================================
  Files         173     130       -43     
  Lines        2612    1807      -805     
  Branches      408     250      -158     
==========================================
- Hits         1179    1030      -149     
+ Misses       1169     653      -516     
+ Partials      264     124      -140

Impacted Files	Coverage Δ
src/universal/styles/withStyles.js	`20% <0%> (-35%)`	⬇️
src/universal/components/Button/Button.js
.../universal/decorators/boundedModal/boundedModal.js
src/universal/utils/getDraftCoords.js
src/universal/styles/helpers/cardRootStyles.js
...rsal/components/ProjectEditor/getAnchorLocation.js
src/universal/utils/draftjs/splitBlock.js
...versal/components/ProjectEditor/withSuggestions.js
...versal/components/MentionBase/mentionBaseStyles.js
src/universal/components/Avatar/Avatar.js
... and 33 more

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 80797c4...1173540. Read the comment docs.

# Conflicts: # src/universal/components/SocketRoute/SocketRoute.js # src/universal/modules/userDashboard/components/UserDashboard/UserDashboard.js # yarn.lock fix Draft.css import

jordanh

Looks great. I suggested a variable name change, but other than that, the code changes look great. Going to run systems tests now...

jordanh · 2017-06-27T13:51:28Z

src/universal/components/Action/Action.js

-        <AsyncRoute
-          mod={() => System.import('universal/components/NotFound/NotFound')}
-        />
+        <AsyncRoute isPrivate path="(/me|/meeting|/newteam|/team)" mod={socketRoute} />


This looks so ✨ clean and shiny✨ !

jordanh · 2017-06-27T13:53:50Z

src/universal/components/AsyncRoute/AsyncRoute.js


 AsyncRoute.propTypes = {
+  bottom: PropTypes.bool,


I'm racking my brain to think of a more descriptive name than bottom. Something that wraps the context that "I'm involved in routing" while still being descriptive. Perhaps, leafRoute would be better?

yeah, i like that.
alternatively, since we have more leaves than not, should we have an isAbstract on everything that isn't a leaf?

Yeah, I like isAbstract. That's cool.

I'd suggest at this line it be:

isAbstractRoute={isAbstract} so it's clearer in bundle Bundle just what that prop is from.

jordanh · 2017-06-27T16:42:15Z

src/universal/containers/Action/ActionContainer.js

-       */
-      updateAnalyticsPage(dispatch, lastPage, nextPage);
-    }
+  componentWillReceiveProps() {


Can you tell me a bit more about this? Why do this instead of setState, for example?

Ah, I see:

finally, this creates a previousLocation singleton that is passed in as context in ActionContainer. This is necessary because segment wants to know the referrer AND the params. Params are not established until it hits the bottom of the routing tree. sending them back to the top a la redux would not work because that would cause a new render cycle, which means lastPage == nextPage. So, sending down the lastPage (now called lastPath since it is a pathname) is necessary.

This should probably be a comment in the code itself

yeah, big gotcha on moving the global from the router (v3) to the context (v4).

jordanh · 2017-06-27T16:52:55Z

Test Cycle Template

It:

sends a page event navigating from My Dashboard to a team
sends a page event navigating from a team to that team's settings
sends a page event navigating from Team Settings to My Dashboard
sends a page events for all steps in a meeting

jordanh · 2017-06-27T16:53:13Z

Test Cycle 1

It:

sends a page event navigating from My Dashboard to a team
sends a page event navigating from a team to that team's settings
sends a page event navigating from Team Settings to My Dashboard
sends a page events for all steps in a meeting

jordanh · 2017-06-27T16:58:25Z

I'm seeing a few things to look into:

The sidebar state doesn't update correctly when I am navigated on a Team Dashboard and I then navigate to My Dashboard
Not all dispatched segmentEventPage events are making it up to segment (I'll look into this one)...

About to make new review

jordanh

Two things to look into:

Check why production builds are failing with Draft.css file in ProjectEditor
Redeploy to development branch and check segment performance using Segment's debug console

Then, I'll retest!

jordanh · 2017-06-27T20:17:33Z

src/universal/components/ProjectEditor/ProjectEditor.js

@@ -11,6 +11,7 @@ import withKeyboardShortcuts from './withKeyboardShortcuts';
 import withLinks from './withLinks';
 import withSuggestions from './withSuggestions';
 import entitizeText from 'universal/utils/draftjs/entitizeText';
+import './Draft.css';


This css file fails a production build. See: https://circleci.com/gh/ParabolInc/action/2904

oh silly me i bet i didn't add it to the prod webpack config

change !bottom to isAbstract for routes add comment explaining previousLocation context

jordanh · 2017-06-27T23:13:10Z

Test Cycle 2

It:

sends a page event navigating from My Dashboard to a team
sends a page event navigating from a team to that team's settings
sends a page event navigating from Team Settings to My Dashboard
sends a page events for all steps in a meeting

jordanh · 2017-06-27T23:15:26Z

Ok, we're getting real close now. When you look at the segment event feed, the page title within the segment is for the previous page event.

jordanh · 2017-06-27T23:21:04Z

Ah shit, I'm guess it's related to this old comment of mine: https://github.com/ParabolInc/action/pull/1088/files#diff-8593bb6594d23b08eb1eb9045737df40L48

jordanh · 2017-06-27T23:24:18Z

src/universal/components/Bundle/Bundle.js

      this.loadMod(nextProps);
    }
+    if (!isAbstractRoute) {


Can this safely be refactored into cDU @mattkrick?

doh! i'm not grabbing it from context here...

er wait...that's right... hmmm why does moving it to cDU help?

mattkrick · 2017-06-28T17:16:23Z

Yeah, you were right that wrapping helmet was the only logical way forward. We still have to call requestIdleCallback to ensure that the children render (and thus change the title) but now title changes happen in sync when the children render. Overall not too ugly

jordanh · 2017-06-29T04:49:47Z

Test Cycle 3

It:

sends a page event navigating from My Dashboard to a team
sends a page event navigating from a team to that team's settings
sends a page event navigating from Team Settings to My Dashboard
sends a page events for all steps in a meeting

jordanh · 2017-06-29T05:33:46Z

Test Cycle 4

It:

sends a page event navigating from My Dashboard to a team
sends a page event navigating from a team to that team's settings
sends a page event navigating from Team Settings to My Dashboard
sends a page events for all steps in a meeting

jordanh

Ok, I caught one little buggy where the name parameter was being passed as undefined. Fixed it, and now the code looks GTG.

jordanh · 2017-06-29T04:52:38Z

src/universal/components/AsyncRoute/AsyncRoute.js

+      exact={exact} path={path}
+      render={({history, location, match}) => (
+        <Bundle
+          isAbstractRoute={isAbstract}


jordanh · 2017-06-29T05:34:37Z

Tests passed!

fix #1086 fix #1047 without sCU

74adbce

jordanh added the pr review label Jun 17, 2017

Merge branch 'draftjs' into segment-hotfix

656ca84

# Conflicts: # src/universal/components/SocketRoute/SocketRoute.js # src/universal/modules/userDashboard/components/UserDashboard/UserDashboard.js # yarn.lock fix Draft.css import

mattkrick changed the title ~~fix #1086 fix #1047 without sCU~~ fix #1086 fix #1047 without sCU [merge after Draftjs] Jun 19, 2017

Merge branch 'draftjs' into segment-hotfix

ad8fdf2

# Conflicts: # src/universal/components/SocketRoute/SocketRoute.js # src/universal/modules/userDashboard/components/UserDashboard/UserDashboard.js # yarn.lock fix Draft.css import

jordanh force-pushed the master branch from 5244913 to a6045fc Compare June 24, 2017 07:00

mattkrick changed the base branch from master to staging June 27, 2017 00:29

mattkrick changed the base branch from staging to master June 27, 2017 00:29

mattkrick changed the title ~~fix #1086 fix #1047 without sCU [merge after Draftjs]~~ re-add segment metadata fix #1086 fix #1047 Jun 27, 2017

mattkrick added 2 commits June 26, 2017 17:37

lint

d3886b3

merge master

d220d80

jordanh previously approved these changes Jun 27, 2017

View reviewed changes

Merge branch 'master' into segment-hotfix

ce22715

jordanh reviewed Jun 27, 2017

View reviewed changes

jordanh added 3 commits June 27, 2017 10:00

added segment to dev if key is configured

7a4a4e6

Merge branch 'master' into segment-hotfix

acec67f

Merge branch 'development' into segment-hotfix

4e990cb

jordanh requested changes Jun 27, 2017

View reviewed changes

mattkrick added 2 commits June 27, 2017 14:36

add css loader to prod build

4a9287f

change !bottom to isAbstract for routes add comment explaining previousLocation context

Merge remote-tracking branch 'origin/segment-hotfix' into segment-hotfix

7750e93

jordanh reviewed Jun 27, 2017

View reviewed changes

fix title for analytics

9c2d5ce

lint

830bafe

fix segment page event name param

1173540

jordanh approved these changes Jun 29, 2017

View reviewed changes

jordanh merged commit 662a6d3 into master Jun 29, 2017

jordanh deleted the segment-hotfix branch June 29, 2017 05:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

re-add segment metadata fix #1086 fix #1047 #1088

re-add segment metadata fix #1086 fix #1047 #1088

mattkrick commented Jun 17, 2017 •

edited

Loading

codecov bot commented Jun 17, 2017 •

edited

Loading

jordanh left a comment

jordanh Jun 27, 2017

jordanh Jun 27, 2017

mattkrick Jun 27, 2017

jordanh Jun 27, 2017

jordanh Jun 27, 2017 •

edited

Loading

jordanh Jun 27, 2017

jordanh Jun 27, 2017

mattkrick Jun 27, 2017

jordanh commented Jun 27, 2017

jordanh commented Jun 27, 2017 •

edited

Loading

jordanh commented Jun 27, 2017

jordanh left a comment

jordanh Jun 27, 2017

mattkrick Jun 27, 2017

jordanh commented Jun 27, 2017

jordanh commented Jun 27, 2017

jordanh commented Jun 27, 2017

jordanh Jun 27, 2017

mattkrick Jun 27, 2017

mattkrick Jun 27, 2017

mattkrick commented Jun 28, 2017

jordanh commented Jun 29, 2017 •

edited

Loading

jordanh commented Jun 29, 2017

jordanh left a comment

jordanh Jun 29, 2017

jordanh commented Jun 29, 2017

re-add segment metadata fix #1086 fix #1047 #1088

re-add segment metadata fix #1086 fix #1047 #1088

Conversation

mattkrick commented Jun 17, 2017 • edited Loading

codecov bot commented Jun 17, 2017 • edited Loading

Codecov Report

jordanh 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

jordanh Jun 27, 2017 • 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

jordanh commented Jun 27, 2017

Test Cycle Template

jordanh commented Jun 27, 2017 • edited Loading

Test Cycle 1

jordanh commented Jun 27, 2017

jordanh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jordanh commented Jun 27, 2017

Test Cycle 2

jordanh commented Jun 27, 2017

jordanh commented Jun 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattkrick commented Jun 28, 2017

jordanh commented Jun 29, 2017 • edited Loading

Test Cycle 3

jordanh commented Jun 29, 2017

Test Cycle 4

jordanh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jordanh commented Jun 29, 2017

mattkrick commented Jun 17, 2017 •

edited

Loading

codecov bot commented Jun 17, 2017 •

edited

Loading

jordanh Jun 27, 2017 •

edited

Loading

jordanh commented Jun 27, 2017 •

edited

Loading

jordanh commented Jun 29, 2017 •

edited

Loading