Adds moment card component #859

dhavalpur0hit · 2018-03-19T07:39:46Z

Creates card component #785

julianguyen · 2018-03-20T23:29:47Z

client/app/bundles/momentCard/components/MomentCardViewers.jsx

+  render() {
+    return (
+      <div>
+        Viewers


You should use the i18n key here

You can omit MomentCardViewersState as the second parameter (it's optional).

On the following lines, you can omit props: MomentCardViewersProp and state: MomentCardViewersState because they should already be defined as part of React.Component< MomentCardViewersProp>.

julianguyen · 2018-03-20T23:30:17Z

yarn-error.log

@@ -0,0 +1,51 @@
+Arguments: 
+  /usr/bin/node /usr/bin/yarn lint:flow


We shouldn't be committing this file!

I added *.log to .gitignore, but you'll want to delete this file.

julianguyen · 2018-03-20T23:30:46Z

app/assets/stylesheets/application/shared.scss

+  font-size: 10px;
+  letter-spacing: 0.1em;
+  text-transform: uppercase;
+  color: rgba(109, 8, 57, 0.5);


This color should be a variable in colors.scss 🌈

julianguyen

Thanks so much for working on this Dhaval! Looking great so far. Let me know if you have any questions about my feedback!

julianguyen · 2018-03-20T23:32:02Z

You'll also want to rebase from master!

nshki · 2018-03-20T23:40:44Z

Thanks a bunch for working on this @dhaval0894! Let me know once you've rebased with master and I'll take a look at everything in-depth.

baohouse · 2018-03-21T01:01:17Z

client/app/stories/index.jsx

@@ -22,6 +22,7 @@ import DropdownFillSmall from '../bundles/shared/components/Dropdown/DropdownFil
 import Footer from '../bundles/shared/components/Footer/Footer';


When you rebase, this file should no longer exist. All the stories are now split up into separate files, so there should be one specifically for the MomentCards.

baohouse · 2018-03-21T01:05:39Z

client/app/bundles/momentCard/components/MomentCardViewers.jsx

+  render() {
+    return (
+      <div>
+        Viewers


You can omit MomentCardViewersState as the second parameter (it's optional).

On the following lines, you can omit props: MomentCardViewersProp and state: MomentCardViewersState because they should already be defined as part of React.Component< MomentCardViewersProp>.

baohouse · 2018-03-21T01:05:57Z

client/app/bundles/momentCard/components/MomentCardSettings.jsx

+};
+
+export default class MomentCardSettings extends
+  React.Component<MomentCardSettingsProp, MomentCardSettingsState> {


You can omit MomentCardViewersState as the second parameter (it's optional).

On the following lines, you can omit props: MomentCardViewersProp and state: MomentCardViewersState because they should already be defined as part of React.Component< MomentCardViewersProp>.

baohouse · 2018-03-21T01:06:45Z

client/app/bundles/momentCard/components/MomentCardName.jsx

+};
+
+export default class MomentCardName extends
+  React.Component<MomentCardNameProp, MomentCardNameState> {


You can omit MomentCardViewersState as the second parameter (it's optional).

On the following lines, you can omit props: MomentCardViewersProp and state: MomentCardViewersState because they should already be defined as part of React.Component< MomentCardViewersProp>.

baohouse · 2018-03-21T01:06:56Z

client/app/bundles/momentCard/components/MomentCardMoods.jsx

+};
+
+export default class MomentCardMoods
+  extends React.Component<MomentCardMoodsProp, MomentCardMoodsState> {


You can omit MomentCardViewersState as the second parameter (it's optional).

On the following lines, you can omit props: MomentCardViewersProp and state: MomentCardViewersState because they should already be defined as part of React.Component< MomentCardViewersProp>.

baohouse · 2018-03-21T01:08:09Z

client/app/bundles/momentCard/components/MomentCardDraft.jsx

+};
+
+export default class MomentCardDraft
+  extends React.Component<MomentCardDraftProp, MomentCardDraftState> {


You can omit MomentCardViewersState as the second parameter (it's optional).

On the following lines, you can omit props: MomentCardViewersProp and state: MomentCardViewersState because they should already be defined as part of React.Component< MomentCardViewersProp>.

baohouse · 2018-03-21T01:08:23Z

client/app/bundles/momentCard/components/MomentCardDate.jsx

+};
+
+export default class MomentCardDate
+  extends React.Component<MomentCardDateProp, MomentCardDateState> {


You can omit MomentCardViewersState as the second parameter (it's optional).

On the following lines, you can omit props: MomentCardViewersProp and state: MomentCardViewersState because they should already be defined as part of React.Component< MomentCardViewersProp>.

baohouse · 2018-03-21T01:08:31Z

client/app/bundles/momentCard/components/MomentCardCategories.jsx

+};
+
+export default class MomentCardCategories
+  extends React.Component<MomentCardCategoriesProp, MomentCardCategoriesState> {


You can omit MomentCardViewersState as the second parameter (it's optional).

On the following lines, you can omit props: MomentCardViewersProp and state: MomentCardViewersState because they should already be defined as part of React.Component< MomentCardViewersProp>.

baohouse · 2018-03-21T01:09:06Z

client/app/bundles/momentCard/components/MomentCard.jsx

+  cardType: string,
+};
+
+export default class MomentCard extends React.Component<MomentCardProp, MomentCardState> {


You can omit MomentCardViewersState as the second parameter (it's optional).

On the following lines, you can omit props: MomentCardViewersProp and state: MomentCardViewersState because they should already be defined as part of React.Component< MomentCardViewersProp>.

baohouse · 2018-03-21T01:12:52Z

client/app/bundles/momentCard/components/MomentCard.jsx

+type MomentCardProp = {
+  item: {
+    name: string,
+    category?: [],


category?: Array<string>, mood?: Array<string>

dhavalpur0hit · 2018-03-21T12:37:50Z

Thanks a lot, @baohouse, @julianguyen, @nshki for taking time to review this. I have updated the PR with the requested changes, so please let me know if there are any further changes. Thanks!

baohouse · 2018-03-21T15:59:26Z

I did a visual inspection.

There's an issue if the main text runs long; it collides with the icons.
The padding on the right should be increased; the icons are too close to the right edge.
Can we setup props that take in onClick handlers for each of the icons?
What is the viewers text for? I did not see it in the mockups.

@nshki How should it behave if the lines run longer than 3 lines? Fixed height with hiding-clipping? Expand the height of the card? Also, is there a minimum width of the card? The mockups showed one card being wider than the others, so is this expandable width-wise?

nshki · 2018-03-21T16:03:50Z

@baohouse Let's expand the height to accommodate for a lot of text.

baohouse · 2018-03-21T19:56:10Z

@nshki I looked at the mockups. With regards to width, it looks like we should make this take up the width of the parent container to be used in grid systems.

nshki · 2018-03-21T19:57:14Z

@baohouse Yup totally!

baohouse · 2018-03-22T05:51:21Z

@dhaval0894

So the card should be fluid; it should take up 100% of the width.
Moment cards should have a min-height, and be allowed to expand to fit content.
When displaying it in Storybook, you can put them inside a grid layout. My PR [WIP] Splitting up Logo file; Adding Ant Design #864 will let us have access to Ant Design; you can look at my Logo story changes there to see how I'm adding a 2-column layout.

dhavalpur0hit · 2018-03-22T06:15:51Z

@baohouse Cool, I'll make these changes.

dhavalpur0hit · 2018-03-23T14:58:04Z

Hey @baohouse, I have made the following changes:

Made the card fluid. Now it will resize depending upon the content.
The main text won't collide with the icons. I have added the max-width and word-wrap property to ensure text gets wrapped if it's too long.
Added onClick handlers for each of the icons.
Wrapped card components in grid layout in the storybook (I had to install ant design package as the PR #864 is not merged into master)
Increased the padding on the right so that the icons won't be too close to the right edge.

Also, the viewers text was already there when I took over the PR from @julianguyen, I didn't know the rationale for that so I kept it :).

dhavalpur0hit · 2018-03-23T15:02:07Z

@baohouse Just for your reference. Please review it and let me know if there are any further changes.

baohouse · 2018-03-27T19:18:59Z

@nshki I see "Viewers" text below the icons. I didn't see this in the original mockups. Was this an ad hoc design change?

nshki · 2018-03-27T19:23:19Z

@baohouse Not sure why the "viewers" text is there. If it or something similar is not in the mocks, it can be removed.

baohouse · 2018-03-28T00:31:55Z

@dhaval0894

Let's remove the viewers text.
Let's make the container display: block and take up the whole width of the parent container

@nshki Do we have a minimum height we want to aim for? I think if we should take the height taken up by two-line title with tags and creation time.

nshki · 2018-03-28T03:35:19Z

@baohouse Yup, that minimum height sounds good to me!

dhavalpur0hit · 2018-03-28T04:12:45Z

@baohouse @nshki Do we really need the min-height? The card adjusts itself depending upon the content height.

baohouse · 2018-03-28T05:23:31Z

@dhaval0894 If you make another story with the 3 card examples sitting on a single row (three column grid), they will all have different heights. I suppose the implementor could override the component CSS to establish a min-height on their own. Very well, not needed.

dhavalpur0hit · 2018-03-28T07:08:59Z

Yes, good point @baohouse. Setting min-height won't help because if the content height is greater than the min-height then also cards will have different height. Like you said, allowing the implementor to override the component CSS to establish a min-height is a good way to do this :)

… locales changes

…bility

dhavalpur0hit · 2018-03-29T15:08:20Z

@nshki @baohouse I have made these changes:

Remove the viewer's text.
Make the container display: block and take up the whole width of the parent container.
Let me know if there are any changes.

baohouse · 2018-03-30T00:38:23Z

@dhaval0894 Mostly looks good. Only thing is that upgrading the parser gem broken bundle install. Can you update it to parser (2.5.0.5)?

dhavalpur0hit · 2018-03-30T14:33:56Z

Hey @baohouse, updated it to parser(2.5.0.5).

nshki

@baohouse covered everything in his review, so I'm also approving as well. :) Thank you both!

baohouse · 2018-03-30T20:53:48Z

@julianguyen I looked at your comments, they've all been addressed. Feel free to approve your requested changes.

julianguyen

Woo thanks so much for working on this Dhaval! Fantastic work :D Thank you @baohouse and @nshki for reviewing this PR so well. So grateful ✨

dhavalpur0hit requested a review from julianguyen March 19, 2018 07:39

dhavalpur0hit force-pushed the moment-card branch 2 times, most recently from a870db7 to adad604 Compare March 20, 2018 08:25

julianguyen requested a review from nshki March 20, 2018 17:53

julianguyen reviewed Mar 20, 2018

View reviewed changes

julianguyen requested changes Mar 20, 2018

View reviewed changes

baohouse suggested changes Mar 21, 2018

View reviewed changes

dhavalpur0hit force-pushed the moment-card branch from adad604 to 8fed86a Compare March 21, 2018 12:32

dhavalpur0hit force-pushed the moment-card branch from 8fed86a to 4a4bdf2 Compare March 23, 2018 12:52

dhavalpur0hit force-pushed the moment-card branch from 5a71ad8 to 68f069f Compare March 27, 2018 09:56

julianguyen and others added 7 commits March 29, 2018 16:41

[WIP] Moment card

3e2dc7d

feature(card): adds moment card component

ef3fab2

fix(rspec tests): updates rspec test corresponding to created, edited…

8ad8f59

… locales changes

fix(moment card): adds unit test for moment card and requested changes

feb67bf

fix(moment card): fixes css issues of the card layout

f569ede

chore(moment card): wraps card in grid layout

b7f146d

fix(Gemfile): updates rails-html-sanitizer version to fix XSS vulnera…

9fe6b0a

…bility

dhavalpur0hit force-pushed the moment-card branch from 68f069f to 9fe6b0a Compare March 29, 2018 14:40

chore(Gemfile): updates parser version to 2.5.0.5

f3c8494

baohouse approved these changes Mar 30, 2018

View reviewed changes

nshki approved these changes Mar 30, 2018

View reviewed changes

julianguyen approved these changes Mar 30, 2018

View reviewed changes

baohouse merged commit 5e269a7 into ifmeorg:master Mar 30, 2018

		@@ -0,0 +1,51 @@
		Arguments:
		/usr/bin/node /usr/bin/yarn lint:flow

		@@ -22,6 +22,7 @@ import DropdownFillSmall from '../bundles/shared/components/Dropdown/DropdownFil
		import Footer from '../bundles/shared/components/Footer/Footer';

Adds moment card component #859

Adds moment card component #859

Conversation

dhavalpur0hit commented Mar 19, 2018

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 commented Mar 20, 2018

nshki commented Mar 20, 2018

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

dhavalpur0hit commented Mar 21, 2018

baohouse commented Mar 21, 2018 • edited Loading

nshki commented Mar 21, 2018

baohouse commented Mar 21, 2018

nshki commented Mar 21, 2018

baohouse commented Mar 22, 2018

dhavalpur0hit commented Mar 22, 2018

dhavalpur0hit commented Mar 23, 2018

dhavalpur0hit commented Mar 23, 2018

baohouse commented Mar 27, 2018

nshki commented Mar 27, 2018

baohouse commented Mar 28, 2018 • edited Loading

nshki commented Mar 28, 2018

dhavalpur0hit commented Mar 28, 2018

baohouse commented Mar 28, 2018

dhavalpur0hit commented Mar 28, 2018

dhavalpur0hit commented Mar 29, 2018

baohouse commented Mar 30, 2018

dhavalpur0hit commented Mar 30, 2018

nshki left a comment

Choose a reason for hiding this comment

baohouse commented Mar 30, 2018

julianguyen left a comment

Choose a reason for hiding this comment

baohouse commented Mar 21, 2018 •

edited

Loading

baohouse commented Mar 28, 2018 •

edited

Loading