Feed: add component #363

layershifter · 2016-08-03T19:02:56Z

WIP

Components
Docs
Tests

Fixes #188

codecov-io · 2016-08-03T19:04:52Z

Current coverage is 94.40% (diff: 100%)

Merging #363 into master will increase coverage by 0.55%

@@             master       #363   diff @@
==========================================
  Files            74         84    +10   
  Lines           960       1055    +95   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits            901        996    +95   
  Misses           59         59          
  Partials          0          0

Powered by Codecov. Last update 012cba6...d34eb88

levithomason · 2016-08-04T21:55:18Z

Note on docs, I've updated the doc example logic and file naming patterns. Doc files are now organized like so:

/docs
  /app
    /Examples
      /views
        index.js  <--- previously "FeedExamples.js"
        /Types
          index.js <-- (optional, can use root index) previously "FeedTypesExamples.js"

You can still put all the examples in the root index.js, just the filename pattern has been updated. I'll be updating the old doc files to use this new naming pattern. All new components should follow it.

layershifter · 2016-08-05T14:38:44Z

@levithomason Did I correctly named doc files?

levithomason · 2016-08-05T15:50:30Z

The naming is only dictated for the index.js files, so you are good. The other files just need to make sense. I think what you have is OK 👍

layershifter · 2016-08-06T20:13:02Z

It seems we're ready for initial discussion 😄

there are huge amount of subcomponents, it's not looks good;
like designed to be used only with like icon, but we use stars in production, so I've made small overhead in <Like>;

<a class="like">
  <i class="like icon"></i> 4 Likes
</a>

I'm using imagePropRender() on Label, it produces html with class="ui image", while there are no classes on <img> in SUI docs;
TAL on <FeedExtra>: seems prop validation for text is ugly, what about shorthand for images, ideas?
seems <Summary> and <Meta> can be shorthands on <Content>, what you think?
do we need render with passing object on <Feed>, like:

const events = [
  { content: 'Text' },
  ...
]

() => <Feed events={events}>

Something else? 😄

levithomason · 2016-08-06T20:16:52Z

@davezuko and I had an offline discussion regarding similar difficulties with sub components in the Message component PR.

I'm out today but will check back tonight or tomorrow and give an update.

levithomason · 2016-08-07T22:25:51Z

there are huge amount of subcomponents, it's not looks good;

Indeed. I'll take any better ideas on how to improve this. Either way, once we get all the components in, we'll revisit these and see if we can optimize them. We may be able to reuse a single set of component parts. I want to get all the components completed with explicit parts before we start to optimize though. This way we can see all the requirements of all the components before making final decisions on how it should be optimized. If you have any ideas or suggestions, please add them to #345 where this is being worked on.

like designed to be used only with like icon, but we use stars in production, so I've made small overhead in <Like>;

Makes sense to me. I think the user should be able to pick the icon as well.

I'm using imagePropRender() on Label, it produces html with class="ui image", while there are no classes on <img> in SUI docs;

It looks like this img does not support any of the <Image /> props nor any classNames (i.e. ui, image, etc.). It is simply an img tag. Because of this, it seems to me we should handle rendering this prop internally in the component. If the prop _.isString() use it as the src for a regular img. If it _.isObject() (an element), use the element itself.

Side note, I plan on improving the icon/prop renderers a bit. Right now they accept either an icon name or image src, or an Icon or Image element. I think it would be good to allow a string (name/src), a plain props object (_.isPlainObject() && !React.isValidElement), or an element (React.isValidElement). I'll probably double back and make these updates a little later. We just need a util to distinguish

TAL on <FeedExtra>: seems prop validation for text is ugly, what about shorthand for images, ideas?

The text prop validation looks correct to me. Can you elaborate on the ugly part you are seeing?

Regarding shorthand for images, we could accept an array of image src strings or image elements. This would make the validation oneOfType([bool, arrayOf(string), arrayOf(node)]).

seems <Summary> and <Meta> can be shorthands on <Content>, what you think?

Makes sense to me. Seems a component is component parts should allow a props to configure those parts.

do we need render with passing object on <Feed>, like:

I also thinks this also makes great sense. Just like <Accordion panels={} />, <Breadcrumb sections={} />, etc.

Something else?

Probably! 😄 This is a really great start though, let's keep momentum and just tackle these items for now.

levithomason · 2016-08-08T05:00:56Z

src/views/Feed/Feed.js

+}
+
+Feed._meta = {
+  library: META.library.semanticUI,


Heads up, I've deprecated library as of today in #380. See that PR for rationale. There is also a failing test for including it now.

layershifter · 2016-08-08T08:16:12Z

It looks like this img does not support any of ...

Updated.

Can you elaborate on the ugly part you are seeing?

Nevermind 😄

Regarding shorthand for images, we could accept an array of image src strings or image elements.

There is problem with key handling. TAL on FeedExtra.js, any ideas?

levithomason · 2016-08-08T16:03:35Z

Sorry, not sure what TAL is and I need a little more info to help on this one. Can you describe the issue with key handling?

layershifter · 2016-08-08T16:07:14Z

src/views/Feed/FeedExtra.js

+  }
+
+  const imagesJSX = images.map((image, index) => {
+    const key = `${index}`


What might be key for image, when image is string? And what when is object?

How about the image src as it is required and will be unique?

In docs src's are same (/images/wireframe/image.png), so I have:

How I can deal with this, when I have element at input?

const images = [<img src='http://semantic-ui.com/images/wireframe/image.png' />, ...];

const imagesJSX = images.map(image => { return _.isString(image) ? <img key={image} src={image} /> : image })

I think that React.cloneElement(image, {key: image.src}) is smelly solution.

Hm, I see.

/cc @davezuko we we're just talking about weak-key, here we have an array of objects with possibly no unique key/value pairs. Ideas for generating unique React keys?

EDIT

I prefer not to add the polyfill for WeakMaps...

layershifter · 2016-08-08T18:22:38Z

Seems, docs and components finished. I'll start writting test after review. So, feedback is welcome 😄

levithomason · 2016-08-09T20:39:29Z

docs/app/Examples/views/Feed/Content/Date.js

@@ -39,7 +39,7 @@ const Date = () => {
        <Feed.Label image={imageSrc} />
        <Feed.Content
          date='3 days ago'
-          summary={<span>You added <a>Jenny Hess</a> to your <a>coworker</a> group.</span>}
+          summary='You added Jenny Hess to your coworker group.'


Reduced all shorthand props, except icon/image, to strings only.

Renders a simple read only feed like so:

levithomason · 2016-08-09T20:44:47Z

Updated, take a look at the last commit and comments.

layershifter · 2016-08-10T16:08:18Z

Thanks for review, all is nice, I'll fix it soon and add tests.

layershifter · 2016-08-11T15:48:18Z

Oh, I missed that you've already applied all 😸 Working on tests, seems we can merge it today.

levithomason · 2016-08-11T17:07:03Z

Awesome, I've got the Modal updates and docs done. It was blocked by the Image, so finishing that up as well.

layershifter · 2016-08-11T18:19:27Z

src/views/Feed/FeedExtra.js

+
+  if (Array.isArray(images)) {
+    const imagesJSX = images.map((image, i) => {
+      return _.isString(image) ? <img key={i} src={image} /> : image


@levithomason It seems that you like antipattern with key more than I 😆

I think:

allow pass only string, not nodes;

use pair of index and src.

Better ideas?

@levithomason It seems that you like antipattern with key more than I 😆

I must admit, I once took it very lightly :S

allow pass only string, not nodes;

use pair of index and src.

There is a chance that the strings might be the same, for instance a placeholder image for multiple images. Also, adding index into the string will affectively do the same thing as having an index only, since the key changes when the child order changes. We would need to accept objects in order to accept a key: images={[{key: 'foo', src: '...'}]}.

I think for now, we allow a string only and fix the duplicate image src string edge case if it arises.

layershifter · 2016-08-11T19:18:01Z

I think I blurted out too much about today 😢
I've finish with tests tomorrow and will wait for final review.

levithomason · 2016-08-11T20:26:42Z

No worries, I'll review once tests are in then. Thanks.

levithomason · 2016-08-13T00:27:50Z

Heads up, I've done some heavy utils refactoring in #388. I left many comments there to help get up to speed. That PR greatly improved things and unblocks the Image and Modal work. Really glad with the results and I think you will be too after updating.

LMK if you have any questions. I'll leave comments on this PR where it needs updated to meet the new refactors later tonight.

levithomason · 2016-08-13T10:49:45Z

I updated this PR to use the new utils. It might be a good idea to familiarize yourself with the changes made in the last commit. I also fixed tests and lint issues.

I'm not sure why PhantomJS is crashing in watch mode though. The branch was doing that before the updates. Master branch does not crash. Looking into it.

levithomason · 2016-08-13T10:54:07Z

Suddenly after the rebase test watching is working again. LMK if you have any issues.

…nologyAdvice/stardust into feature/feed

layershifter · 2016-08-13T16:28:00Z

Suddenly after the rebase test watching is working again.

Offtop: Did you have issues with PhantomJS's crash, too?

Feed.js

I've updated key prop handling as we discussed.

const eventsJSX = events.map(eventProps => {
    const { childKey, date, meta, summary, ...eventData } = eventProps
    const finalKey = childKey || `${date}-${meta}-${summary}`

    return <FeedEvent
      date={date}
      key={finalKey}
      meta={meta}
      summary={summary}
      {...eventData}
    />
  })

FeedExtra.js

Only array of string can be passed now.

- PropTypes.arrayOf(PropTypes.node),
+ PropTypes.arrayOf(PropTypes.string),

Updated key prop (better ideas?) and used new factory.

const imagesJSX = images.map((image, index) => {
  const key = [index, image].join('-')

  return createImg(image, {key})
})

FeedLabel-test.js

Are tests for image prop correct (createImg factory)?

Tests are ready. Show must go on 👻

levithomason · 2016-08-13T19:34:11Z

Did you have issues with PhantomJS's crash, too?

Indeed, not sure what fixed it in the rebase but it seems to be fine now?

I've updated key prop handling as we discussed.

We'll have to pick a different prop name as key won't work, see here: https://github.com/TechnologyAdvice/stardust/pull/390#discussion_r74691048

levithomason · 2016-08-13T19:35:41Z

src/views/Feed/Feed.js

      date={date}
      key={finalKey}
      meta={meta}
      summary={summary}
      {...eventData}
-    />
+            />)


Indent issue here, needs some new lines inserted.

levithomason · 2016-08-13T19:37:44Z

FeedExtra 👍

FeedLabel-test 👍 though, we'll need to update the key prop to another name then update the factories to apply our standard key prop name to the key prop.

layershifter · 2016-08-13T20:27:12Z

Indeed, not sure what fixed it in the rebase but it seems to be fine now?

Yep.

We'll have to pick a different prop name as key won't work, see here: #390 (comment)

events: customPropTypes.every([
  PropTypes.arrayOf(PropTypes.shape({
    childKey: PropTypes.string,
  })),
]),

childKey will be applied to FeedEvent as it might be, or not?

we'll need to update the key prop to another name then update the factories to apply our standard key prop name to the key prop.

??? Not understand you there.

levithomason · 2016-08-13T20:36:51Z

What you've done for FeedEvent is correct. My comment was in regard to passing a childKey prop to the image prop:

// on an element
const image = <img childKey='...' src='...' />
<FeedLabel image={image} />

// or a props object (this is a new feature of our factories)

<FeedLabel image={{ childKey='...', src='...' }}

Our factories are responsible for merging those props by default. Currently, they will simply pass the childKey prop since onto the final element. We need to map childKey to the key prop for the final element in the createFactory.js mergePropsAndClassName().

This work can be done on a separate PR though to keep this one moving.

levithomason · 2016-08-13T20:37:45Z

Anything else to update here? Looks great. I'd like to merge and make any updates on future PRs.

layershifter · 2016-08-13T20:52:16Z

No, we can go 🚗

levithomason · 2016-08-13T21:17:03Z

💥 Released in stardust@0.34.0

* feat(Feed) Initial commit * feat(Feed) Docs and components update * feat(Feed) Docs and components update * feat(Feed) More docs and components * feat(Feed) Remove meta.library * feat(Feed) Update prop handling * feat(Feed) Update prop handling * feat(Feed) More shorthands * refactor(Feed): use string shorthand props * feat(Feed) Update prop handling and tests * fix(Feed) More tests * refactor(Feed): update to refactored utils * fix(Feed) Fixes and more tests * fix(Feed) Lint issues * fix(Feed) Update lint issues

levithomason reviewed Aug 8, 2016
View reviewed changes

layershifter reviewed Aug 8, 2016
View reviewed changes

levithomason force-pushed the feature/feed branch from 5628698 to 5276567 Compare August 9, 2016 20:38

levithomason reviewed Aug 9, 2016
View reviewed changes

layershifter reviewed Aug 11, 2016
View reviewed changes

Alexander Fedyashov and others added 10 commits August 13, 2016 01:23

feat(Feed) Docs and components update

22bf1c8

feat(Feed) More docs and components

8df7077

feat(Feed) Remove meta.library

a3fce64

feat(Feed) Update prop handling

cd3c201

feat(Feed) Update prop handling

e46ddd6

feat(Feed) More shorthands

a68c759

refactor(Feed): use string shorthand props

b362097

feat(Feed) Update prop handling and tests

1d0e556

fix(Feed) More tests

1d31612

refactor(Feed): update to refactored utils

51da3e3

levithomason force-pushed the feature/feed branch from 774d504 to 51da3e3 Compare August 13, 2016 10:45

layershifter added 2 commits August 13, 2016 18:46

Merge branches 'feature/feed' and 'master' of https://github.com/Tech…

379da0f

…nologyAdvice/stardust into feature/feed

fix(Feed) Fixes and more tests

aa3dd4b

fix(Feed) Lint issues

43f33d6

levithomason reviewed Aug 13, 2016
View reviewed changes

fix(Feed) Update lint issues

d34eb88

levithomason merged commit 6cfa43e into master Aug 13, 2016

levithomason deleted the feature/feed branch August 13, 2016 21:15

levithomason changed the title ~~Add Feed component~~ Feed: add component Aug 18, 2016

Feed: add component #363

Feed: add component #363

Conversation

layershifter commented Aug 3, 2016 • edited Loading

WIP

codecov-io commented Aug 3, 2016 • edited Loading

Current coverage is 94.40% (diff: 100%)

levithomason commented Aug 4, 2016 • edited Loading

layershifter commented Aug 5, 2016

levithomason commented Aug 5, 2016

layershifter commented Aug 6, 2016

levithomason commented Aug 6, 2016

levithomason commented Aug 7, 2016

Choose a reason for hiding this comment

layershifter commented Aug 8, 2016 • edited Loading

levithomason commented Aug 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

levithomason Aug 8, 2016 • edited Loading

Choose a reason for hiding this comment

layershifter commented Aug 8, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

levithomason commented Aug 9, 2016

layershifter commented Aug 10, 2016

layershifter commented Aug 11, 2016

levithomason commented Aug 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

layershifter commented Aug 11, 2016

levithomason commented Aug 11, 2016

levithomason commented Aug 13, 2016

levithomason commented Aug 13, 2016

levithomason commented Aug 13, 2016

layershifter commented Aug 13, 2016

Feed.js

FeedExtra.js

FeedLabel-test.js

levithomason commented Aug 13, 2016

Choose a reason for hiding this comment

levithomason commented Aug 13, 2016

layershifter commented Aug 13, 2016

levithomason commented Aug 13, 2016

levithomason commented Aug 13, 2016

layershifter commented Aug 13, 2016

levithomason commented Aug 13, 2016

layershifter commented Aug 3, 2016 •

edited

Loading

codecov-io commented Aug 3, 2016 •

edited

Loading

levithomason commented Aug 4, 2016 •

edited

Loading

layershifter commented Aug 8, 2016 •

edited

Loading

levithomason Aug 8, 2016 •

edited

Loading

layershifter commented Aug 8, 2016 •

edited

Loading