Refactored ProfilePicture to use React component #876

baohouse · 2018-04-01T02:43:34Z

Addresses #874

Change overview:

Updated ProfilePicture class to use Avatar React component.
Enhanced Avatar component to use react-lazyload
Updated tests that assert on the ProfilePicture
Fixed some tests for language toggling involving page transitions

PR checklist:

yarn test
yarn lint
rspec
rubocop
Manual inspection of if me homepage for visual parity
Manual check for lazy loading of images to make sure the images do not load until the user scrolls them into view
Manual check that image size on if me homepage is correct in both desktop and mobile widths
Manual inspection on if me profile page for visual parity

1. Updated ProfilePicture class to use Avatar React component. 2. Enhanced Avatar component to use react-lazyload 3. Updated tests that assert on the ProfilePicture 4. Fixed some tests for language toggling involving page transitions

julianguyen · 2018-04-03T06:09:43Z

client/app/bundles/shared/components/_colors.scss

+ * This file contains the color palette for the Kujali interface.
+ */
+
+$theme-color-1: #6d0839;


Rename as $theme-color-primary? Really though it should be colour 🌈

We can call it primary, that's fine too. It's possible in the future that a theme has more than 2 colors, so that's why I just used straight up numbers. In the future, you can then assign variables:

$text-highlight: $theme-color-secondary; mark { color: $text-highlight }

"Colour"? Haha, I would say CSS use of "color" makes things more consistent, but, if you really want, I can make the change. 😆

I think if there's more than two colours in the future, we should just give them straight up names

julianguyen · 2018-04-03T06:09:58Z

client/app/bundles/shared/components/_colors.scss

+ */
+
+$theme-color-1: #6d0839;
+$theme-color-2: #d0e799;


Rename as $theme-color-secondary?

julianguyen · 2018-04-03T06:11:31Z

client/app/stories/Avatar.stories.jsx

+  .add('With name', withSource(
+    <Row style={{ backgroundColor: '#aaa' }}>
+      <Col span={8}>
+        <Avatar src={photoJulia} name="Julia &ldquo;Fleurchild&rdquo; Nguyen" displayname />


LOL 🌷👧

Haha, I'm just testing to see what the component looks like when the line length exceeds the width of the image. Honest!

julianguyen · 2018-04-03T06:12:34Z

client/karma.conf.js

+    return specList.split(',');
+  } else {
+    return [
+      { pattern: 'app/**/*.spec.js', watched: true },


Hmm not 100% sure why it's broken up like this, can you explain? Thanks!

In the client/README.md I documented a way to run specific tests. I looked up on Stackoverflow on how to do this, and this was the best answer I could find so far. So if you specify a list in the environment properties, it will run those, otherwise falling back to all tests.

julianguyen · 2018-04-03T06:14:46Z

app/assets/stylesheets/application.scss

@@ -593,22 +586,30 @@ img {
 }

 .small_profile_picture_div {
-  width: 100px;
-  height: 100px;
+  width: 1px;


Why is this 1px?

This is a div element made to behave like a table cell. Since we want to control the width by way of the content's width (which in this case is the Avatar component), we can reduce the number of places to update width information from two places to one.

julianguyen · 2018-04-03T06:15:02Z

app/assets/stylesheets/application.scss

  }
 }

 .profile_picture_div {
-  width: 150px;
-  height: 150px;
+  width: 1px;


Similar question as above ^

Ditto as above 😄

julianguyen

Thanks a million for working on this <3

I have a handful of questions that need answering, but overall, LGTM! Excited that we're starting to see put together the React and Rails sides.

julianguyen

Thanks for answering my questions and working on this! LGTM!

nma · 2018-04-05T04:16:46Z

client/webpack.config.js

@@ -1,3 +1,8 @@
+/**
+ * TODO: Split up development configuration from production configuration


it is a bit more involved than that sadly, we will need to move react_on_rails to v9 and bump us up to webpacker instead of webpacker_lite gem

nma · 2018-04-05T04:20:30Z

client/webpack.config.js

@@ -35,11 +40,13 @@ const config = Object.assign(baseConfig, {
  context: resolve(__dirname),

  entry: {
-    'webpack-bundle': [
+    'moments-app_bundle': glob.sync('./app/bundles/momentsApp/startup/*'),


will we be separating bundles for momentsApp / alliesApp and etc going forward? Or only moments app due to the chart.js requirements?

We should let webpack determine chunking. If we use UI routing, then this becomes far more feasible. Right now we're manually creating chunk entries, but that's not scalable. So we should move away from this. But in all honesty, I think the concept of a bundle makes more sense if it was Origin UI vs Kujali UI. When you have a MomentCard in the momentsApp bundle needing a component from the shared bundle, you can achieve the same effect if we put all components into a single folder and make two registration files, one for MomentsApp and one more Common. Or, well, like I said first, let webpack determine the chunking per route.

nma · 2018-04-05T04:24:29Z

app/views/layouts/application.html.erb

+    <%= stylesheet_pack_tag 'shared_bundle' %>
+
+    <!-- TODO: Need a way to load this bundle only on Moments pages -->
+    <%= javascript_pack_tag 'moments-app_bundle' %>


my thoughts here is that once we move the entirety of the app to React, we can just inject one bundle. This feels like a transition code atm.

Refactored ProfilePicture to use React component

31bafad

1. Updated ProfilePicture class to use Avatar React component. 2. Enhanced Avatar component to use react-lazyload 3. Updated tests that assert on the ProfilePicture 4. Fixed some tests for language toggling involving page transitions

baohouse requested review from GeoDoo and julianguyen April 1, 2018 02:43

baohouse and others added 5 commits March 31, 2018 20:00

Fixing CodeClimate complaints

894e96d

Fixing typo

0aabf06

Reorganizing MomentsApp bundle

c995620

Fixing lint errors and paths

9023e14

Updating tests to asseto on Hash

27c1c65

julianguyen reviewed Apr 3, 2018

View reviewed changes

baohouse requested a review from nma April 3, 2018 22:26

baohouse added 4 commits April 3, 2018 16:35

Updating flow handling of assets

c12f927

Fixing lint ignore

48f5c7a

Adding ignore path for CodeClimate

8aa53ab

Merge branch 'master' into avatar

d7828d6

julianguyen approved these changes Apr 5, 2018

View reviewed changes

nma reviewed Apr 5, 2018

View reviewed changes

Merge branch 'master' into avatar

2ebeccd

baohouse merged commit d8c4a98 into ifmeorg:master Apr 5, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactored ProfilePicture to use React component #876

Refactored ProfilePicture to use React component #876

baohouse commented Apr 1, 2018 •

edited

Loading

julianguyen Apr 3, 2018

baohouse Apr 3, 2018

julianguyen Apr 5, 2018

julianguyen Apr 3, 2018

julianguyen Apr 3, 2018

baohouse Apr 3, 2018

julianguyen Apr 3, 2018

baohouse Apr 3, 2018

julianguyen Apr 3, 2018

baohouse Apr 3, 2018

julianguyen Apr 3, 2018

baohouse Apr 3, 2018

julianguyen left a comment

julianguyen left a comment

nma Apr 5, 2018

nma Apr 5, 2018

baohouse Apr 5, 2018 •

edited

Loading

nma Apr 5, 2018

		@@ -1,3 +1,8 @@
		/**
		* TODO: Split up development configuration from production configuration

Refactored ProfilePicture to use React component #876

Refactored ProfilePicture to use React component #876

Conversation

baohouse commented Apr 1, 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

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

julianguyen left a comment

Choose a reason for hiding this comment

julianguyen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

baohouse Apr 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

baohouse commented Apr 1, 2018 •

edited

Loading

baohouse Apr 5, 2018 •

edited

Loading