Look away #314

mattkrick · 2016-10-03T06:11:56Z

HOLY COW APHRODITE IS SO FAST

This has the latest from the me-dash-dos branch
it also fixes the welcome wizard lag. and a lot more.
I also refactored the SSR, cuz it was ugly.

The main thing it does is switch from react-look to aphrodite.
I created a withStyles HOC that takes in the styles & spits out the component.
That way we can have user-specific themes in the future (see ThemeProvider)
It also means we can still use props to determine our styles (see Button).
It still only calls aphrodite once per component instantiation, which is something that https://github.com/airbnb/react-with-styles/tree/master/src doesn't do. I wish I could have used it. Airbnb usually puts out pretty good stuff. But their version complicates the heck outta things, introduces new singletons, & is much less performant. Their API was nice though, so I used that.

I did have to write a package for aphrodite to handle globals, but hopefully they see how simple it is & accept my PR, too.

I'm gonna wake up late tomorrow.

…sh-dos

# Conflicts: # src/universal/modules/userDashboard/components/UserActionList/UserActionList.js # src/universal/modules/userDashboard/components/UserActionList/UserActionListItem.js

mattkrick · 2016-10-03T15:41:12Z

Fix list (to be added when merging!)
fix #313
fix #307
fix #305
fix #302
fix #295
fix #290
fix #282
fix #276
fix #252
fix #227
fix #221
fix #190
fix #124

jordanh

My god, this was such an awesome amount of work. Bravo!!

Please see the one change I'm requesting...

jordanh · 2016-10-03T19:22:07Z

package.json

@@ -16,7 +16,8 @@
  "scripts": {
    "start": "NODE_ENV=production node ./src/server/server.babel.js",
    "dev": "NODE_ENV=development node ./src/server/server.babel.js",
-    "build": "rimraf build && concurrently \"npm run build:client\" \"npm run build:server\"",
+    "build:themes": "babel-node ./webpack/utils/buildThemeJSON.js",


Did you prefer building the theme here over using WebpackShellPlugin? I'm cool with this of course, just curious as to why...

Oh that was a fun one!
I'm not sure I completely understand it, but here goes...
I was getting an error in the json loader where it was getting an empty string, which is what happens if you read an empty file. Maybe 20% of the time, only when the build folder existed.
I think that's because the server and client builds were both building that file. What I think was happening is that one would try to read the file before the other one built it, thanks to concurrently.

Ah yes, that is why I changed this build to be serial (not concurrent) at one point. I like your solution better.

@ackernaut what do you think here?

I'm not sure I follow 100% on how the theme is building. Wasn’t it a matter of calculating the values on the fly in dev and building an object in production? Or am I totally lost?

Sorry, wrong comment box! Heh got too many.

jordanh · 2016-10-03T19:27:21Z

src/universal/components/OutcomeCard/OutcomeCardFooter.js

              name="ellipsis-v"
              style={menuHintStyle}
            />
          }
        </div>
      </div>
-      <div className={styles.buttonBlock}>
+      <div className={css(styles.buttonBlock)}>
        <button className={buttonStyles} onClick={handleStatusClick}>


Left a similar comment on the last PR: when this footer renders on the /me dashboard, a click on this should raise a menu to change the project's team, not the project's owner. I think we've got just a little bit more to go here...

You want projects to be able to change teams?? Doing so is not trivial...

Yes; I think that's the design intent (but we can talk about it). Right now, it's strange to click on a team and have it pop user avatars. Perhaps for now we just disable the click handler on the project on the /me? Good enough for alpha? What do you think?

Ah yeah, we had that, but seeing your face 50 times was kinda weird, so me n Terry figured we'd show the team, but let them change the team member. Since team ID is built into the immutable primary key, wed essentially delete and recreate. Changing teams also means that we can't easily see the change history of a project due to permissions. I'm cool with whatever

I meant to put this here. @ackernaut what are your thoughts here?

I do have a thought here! :) So the latest card design has a hover state (vertical ellipsis) for the avatar:name block indicating you can click on it. The other components have those subtle hints, too. For user dash, let’s just make it a static label with the team name and give no indication of interact-ability. It will be like the action list items, in a sense. Yay, nay?

ackernaut · 2016-10-03T19:29:22Z

I’m getting an issue when trying to archive a project card:

Uncaught TypeError: Cannot read property 'startsWith' of undefined ProjectCardContainer.js:51

mattkrick · 2016-10-03T20:01:12Z

@ackernaut I noticed that once, but couldn't reproduce it. Do you have some reliable steps to reproduce it?

ackernaut

Oh my! Such a good PR! Super-human.

I left a few notes. I think I may have answered a few questions after reviewing further.

I think there is some newer work that didn’t make it over from the summary branch I was working on. I can try merging that in, too (and updating to Aphrodite). Do you mind if I push in any small changes?

ackernaut · 2016-10-03T19:44:24Z

src/universal/components/Ellipsis/Ellipsis.js

-    dotStyles3 = combineStyles(s.dotAnimated, s.dot3);
-  }
-
+  // TODO only use 1 style?


@mattkrick we need to figure out a way to alternate the timing of each dot or change the design of the animation. As written all three dots pulse as the same time.

whoops. fixed in the next commit

ackernaut · 2016-10-03T19:47:55Z

src/universal/components/IconButton/IconButton.js

    </button>
  );
 };

-styles = StyleSheet.create({
+IconButton.propTypes = {


Combine this with the propTypes object toward the end of the document?

fantastic eye for detail. fixed!

ackernaut · 2016-10-03T19:51:16Z

src/universal/components/IconLink/IconLink.js

@@ -96,6 +69,7 @@ IconLink.propTypes = {
    'small',
    'large'
  ]),
+  styles: PropTypes.object,
  theme: PropTypes.oneOf([


Does this prop name need to change to colorPalette ?

ackernaut · 2016-10-03T19:53:30Z

src/universal/components/InputField/InputField.js

    hasErrorText,
    hasHelpText,
    helpText,
    input,
    isWider,
-    theme,
+    theme: inputTheme,


Should it just be a defaultProp?

ah, I'll change this to colorPalette.
defaultProps are ok in this case, but they aren't passed to the withStyles HOC, which means if our stylesThunk depends on the colorPalette, it'll be undefined. For that reason, I try to avoid defaultProps & explicitly pass that in, but I don't see a single case where anything using InputField passes in that value.

ackernaut · 2016-10-03T19:53:44Z

src/universal/components/InputField/InputField.js

+    styles,
+  } = props;
+
+  const styleTheme = inputTheme || 'cool';


Should it just be a defaultProp?

yeah, let's do that for now

ackernaut · 2016-10-03T20:29:43Z

src/universal/modules/patterns/components/Example/Example.js

-    padding: '2rem'
-  }
-});
+// import React, { Component, PropTypes } from 'react';


Not currently being used, but was originally intended for examples in the patterns view.

ah! i wasn't sure, so i just commented out a bunch of things that weren't in the app. what should i do here?

I don’t mind yanking the components in patterns. I do think the shortcuts menu is still something we want to work out, perhaps move that to /meeting module instead of /team module (does that module even get used?)

ackernaut · 2016-10-03T20:33:45Z

src/universal/modules/team/components/ShortcutsMenu/ShortcutsMenu.js

-});
-
-export default look(ShortcutsMenu);
+// import React, {PropTypes} from 'react';


Don’t want to lose the shortcuts menu, but it hasn’t been implemented yet.

ackernaut · 2016-10-03T21:59:27Z

I’m noticing some differences in how the type is rendering. I wonder if that is simply because of the change in format (to woff) or something else is also going on. Looking into it.

mattkrick · 2016-10-03T22:26:33Z

@ackernaut oh that's interesting! i'll commit the ttf versions and we can play with it.

ackernaut · 2016-10-03T22:32:52Z

@mattkrick I wonder if my console error is because I am seeded inactive on the Engineering team?

ackernaut · 2016-10-03T22:33:11Z

@mattkrick I have some style updates that I can merge here (my latest summary work). Whatcha think?

mattkrick · 2016-10-03T22:34:30Z

@ackernaut yeah, we can add em here or we can merge the summary branch onto this one, whichever you think is least painful

ackernaut · 2016-10-03T22:36:51Z

@mattkrick I basically did that, I branched from here, merged in summary, fixed conflicts, and pushed as look-away-summary which should fit nicely here.

mattkrick · 2016-10-03T22:51:19Z

super! anything else I need to do before we merge this sucker?

ackernaut · 2016-10-03T22:55:42Z

Let me merge my branch real quick...

ackernaut and others added 30 commits September 26, 2016 17:35

Just getting started with Summary UI components

15f5c63

First pass, UserActionList sub-components

3cc222f

More pixel-tweaking for list header control

2f6d5c4

More pixel-tweaking for list header control

5cbb922

Tried to ammend, meh

f4040bd

Handling various states of action list

562aea4

fix #302

03a11fe

update users preferredName from UserHub (fixes #290)

3f5b3f5

Yank timestamp of action list item

40656e5

Icon gutter

405ff4e

wire up SelectTeam

55c885f

Merge remote-tracking branch 'origin/me-dash-dos' into me-dash-dos

348b04a

refactor OutcomeCardTextarea to use with actions

32b2c27

refactor OutcomeCardTextarea styles

09571ab

Merge branch 'master' into summary-ui-part1

5e23a19

Progress on summmary email

00c95f3

fix adding actions

36395f6

Menu and MenuToggle with demo

e7ad1c2

Merge branch 'me-dash-dos' of github.com:ParabolInc/action into me-da…

89deb51

…sh-dos

Move demo array

91a99ef

fix add action header

855513d

Merge remote-tracking branch 'origin/me-dash-dos' into me-dash-dos

dd58223

AgendaList now fills the column height, scrollable panel

bd1fe45

Minor clean-up

bb5c5c6

now with completeable actions!

e4882dc

Merge remote-tracking branch 'origin/me-dash-dos' into me-dash-dos

dd04439

# Conflicts: # src/universal/modules/userDashboard/components/UserActionList/UserActionList.js # src/universal/modules/userDashboard/components/UserActionList/UserActionListItem.js

Latest summary email

a799b49

userDash now has (buggy) menus

686d516

Updates to menu, fixes column layout

5ea56d6

Tweak menu width for min-width column sizing

2405eef

jordanh added the pr review label Oct 3, 2016

mattkrick added 2 commits October 3, 2016 06:55

update readme to aphrodite

3336529

use hepha instaed of aphrodite fork

9ee0ffe

fix #276

68cd3fd

jordanh requested changes Oct 3, 2016

View reviewed changes

ackernaut reviewed Oct 3, 2016

View reviewed changes

Small fixes

61196c4

mattkrick added 2 commits October 3, 2016 15:33

terry fixes

3c557ad

Merge remote-tracking branch 'origin/look-away' into look-away

95db432

mattkrick and others added 6 commits October 3, 2016 16:42

terry active link fix

6e7cca0

Merge from summary branch

715acb3

Fix linting, etc

c8e4757

More patch fixes

72d09c0

More fixes, back in

74e6bf2

Disable routes

05a1622

jordanh approved these changes Oct 4, 2016

View reviewed changes

jordanh merged commit 681a5dd into master Oct 4, 2016

jordanh removed the pr review label Oct 4, 2016

jordanh deleted the look-away branch October 4, 2016 14:35

Look away #314

Look away #314

Conversation

mattkrick commented Oct 3, 2016

mattkrick commented Oct 3, 2016 • edited Loading

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

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

ackernaut commented Oct 3, 2016

mattkrick commented Oct 3, 2016

ackernaut 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

ackernaut commented Oct 3, 2016

mattkrick commented Oct 3, 2016

ackernaut commented Oct 3, 2016

ackernaut commented Oct 3, 2016

mattkrick commented Oct 3, 2016

ackernaut commented Oct 3, 2016

mattkrick commented Oct 3, 2016

ackernaut commented Oct 3, 2016

mattkrick commented Oct 3, 2016 •

edited

Loading