feat(Advertisement): add new component #1292

tarang9211 · 2017-02-06T16:49:26Z

Fixes #185

codecov-io · 2017-02-06T19:33:37Z

Codecov Report

Merging #1292 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1292      +/-   ##
==========================================
+ Coverage   99.74%   99.74%   +<.01%     
==========================================
  Files         140      141       +1     
  Lines        2347     2354       +7     
==========================================
+ Hits         2341     2348       +7     
  Misses          6        6

Impacted Files	Coverage Δ
src/views/Advertisement/Advertisement.js	`100% <100%> (ø)`

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 b12a368...d3ec665. Read the comment docs.

tarang9211 · 2017-02-06T19:38:59Z

Can someone giving feedback on this? I have 4 tests that need to pass, also need more info about the workings of this component.

@layershifter or @levithomason could you give some feedback?

layershifter

@tarang9211 I've left some comments, but all of them is about styling. You're on right way 👍

I could say more on the case, after you will add the some examples in the docs.

layershifter · 2017-02-07T08:06:59Z

src/index.js

@@ -139,6 +139,8 @@ export { default as SidebarPushable } from './modules/Sidebar/SidebarPushable'
 export { default as SidebarPusher } from './modules/Sidebar/SidebarPusher'

 // Views
+export { default as Advertisement } from './views/Advertisement/Advertisement'
+


Unnecessary blank line.

layershifter · 2017-02-07T08:07:49Z

src/views/Advertisement/Advertisement.js

+} from '../../lib'
+
+/**
+*An ad displays third-party promotional content


Missing gap and dot:

- *An ad displays third-party promotional content +* An ad displays third-party promotional content.

layershifter · 2017-02-07T08:08:28Z

src/views/Advertisement/Advertisement.js

@@ -0,0 +1,40 @@
+import React, { PropTypes } from 'react'
+import cx from 'classnames'


We sort imports in last PRs:

-import React, { PropTypes } from 'react' -import cx from 'classnames' +import cx from 'classnames' +import React, { PropTypes } from 'react'

layershifter · 2017-02-07T08:09:28Z

src/views/Advertisement/Advertisement.js

+  )
+  const ElementType = getElementType(Advertisement, props)
+
+  return <ElementType className={classes} />


I think that this component will have children?

Also, to get test working you need use there getUnhandledProps function and spread rest to ElementType.

And you need to add _meta of course.

I added the meta property here: https://github.com/tarang9211/Semantic-UI-React/blob/5e34b354fcbd0a9da6847893c96172bac4564dee/src/views/Advertisement/Advertisement.js#L27 I was referring to the Feed component and structuring it that way. I'll push a new commit with your changes.

What should be the children of this component? If I had the line below, about 11 tests fail

if (!_.isNil(children)) { return <ElementType {...rest} className={classes}>{children}</ElementType> }

tarang9211 · 2017-02-07T09:30:28Z

@layershifter pushed a new commit with your changes. Here's a screenshot of the test that fails.

Im defining the _meta property as per this - https://github.com/Semantic-Org/Semantic-UI-React/blob/master/src/views/Statistic/Statistic.js#L58

layershifter · 2017-02-07T10:07:02Z

I've pushed some commits, tests are passed 👍

tarang9211 · 2017-02-07T19:35:11Z

@layershifter checked them out right now. Don't we also need a nil check for children?

tarang9211 · 2017-02-08T07:36:02Z

@layershifter could we create like a to-do for this task? That would help knocking things off. Let me know your thoughts.

layershifter · 2017-02-08T07:58:35Z

Don't we also need a nil check for children?

Nope.

could we create like a to-do for this task?

Let's implement examples from SUI docs.

tarang9211 · 2017-02-08T11:55:26Z

@layershifter hmm alright. So are we handling the various sizes via props?

tarang9211 · 2017-02-08T13:04:17Z

Let me know if you think these are on point and what you're looking for!

layershifter · 2017-02-08T13:25:37Z

@tarang9211 I'll take a look today.

layershifter · 2017-02-08T14:17:25Z

Okay, as I see we have following units:

(mobile, large) leaderboard;
(medium, large) rectangle;
half page;
(small) square;
(small, vertical) rectangle;
(small, square) button;
(wide) skyscrape;
(vertical, top, half, large mobile) banner;
billboard;
panorama;
netboard.

Other props:

centered;
test.

Thoughts

All unit props will be separate:

leaderboard: PropTypes.oneOfType(
  PropTypes.bool,
  PropTypes.oneOf(['mobile', 'large'])
),
panorama: PropTypes.bool,

Also, as I see we cannot combile units, so they must also be single:

panorama: customPropTypes.every([
  customPropTypes.disallow(['leaderboard']),
  customPropTypes.disallow(['button']),
  // ...
  PropTypes.node,
])

Test will be combo-prop:

test: PropTypes.oneOfType(
  PropTypes.bool,
  PropTypes.number,
  PropTypes.string,
)

<Ad test/> // => <div class="ui test ad"></div>
<Ad test='hi' /> // => <div class="ui test ad" data-text="Ad Unit 2"></div>

@levithomason Other ideas?

tarang9211 · 2017-02-08T18:10:12Z

Thanks for the input I'll get to this in a few

jeffcarbs · 2017-02-08T18:34:34Z

@layershifter - Given that the different "units" only affect height/width and thus can't be combined, I consider "unit" a single prop with different possible values. I think it should be similar to https://github.com/Semantic-Org/Semantic-UI-React/blob/master/src/collections/Grid/Grid.js#L121-L123

I took the values from the CSS file you linked, so it'd be:

unit: PropTypes.oneOf([
  'banner', 'half banner', 'large mobile banner' 'top banner', 'vertical banner'
  'billboard',
  'button', 'square button', 'small button',
  'half page',
  'small rectangle', 'medium rectangle', 'large rectangle', 'vertical rectangle',
  'leaderboard', 'large leaderboard', 'mobile leaderboard',
  'netboard',
  'panorama',
  'skyscraper', 'wide skyscraper',
  'square', 'small square'
])

layershifter · 2017-02-08T18:51:51Z

@jcarbo good suggestion, I like it 👍

tarang9211 · 2017-02-08T18:52:44Z

Pushing a commit with @jcarbo's suggestions!

Could someone explain what test prop is really? Right now I'm seeing this on my local doc site examples and I don't know where it gets the Ad text from.

tarang9211 · 2017-02-14T07:39:00Z

@levithomason let me know what you think of the changes I made.

tarang9211 · 2017-02-14T16:26:21Z

Any idea about my concern regarding the sizing issue?

…

On Tue, Feb 14, 2017 at 9:53 PM Levi Thomason ***@***.***> wrote: ***@***.**** approved this pull request. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1292 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFmqGUTK2S__pAM9aVtY1Okdp0B_wwgHks5rcdUHgaJpZM4L4cJu> .

levithomason · 2017-02-14T16:28:07Z

Checking into it, I think it is style related.

Also, the ad only loads on page refresh. If you navigate from, say the button docs, to the advertisement docs, the ad doesn't load (at least for me)

tarang9211 · 2017-02-14T16:29:27Z

Same for me. I noticed the same thing

…

On Tue, Feb 14, 2017 at 9:58 PM Levi Thomason ***@***.***> wrote: Checking into it, I think it is style related. Also, the ad only loads on page refresh. If you navigate from, say the button docs, to the advertisement docs, the ad doesn't load (at least for me) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1292 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFmqGRdIVWF7I9Z6ahB5VL9EEo3G6XKMks5rcdYbgaJpZM4L4cJu> .

levithomason · 2017-02-14T16:32:39Z

I think the size issue is due to the style prop, using this style prop from the SUI core example:

style={{ display: 'inline-block', width: 300, height: 250 }}

I get these results:

I am not sure on the mounting problem.

levithomason · 2017-02-14T16:34:05Z

Unfortunately, that's all the time I have for now on this one. I'll check back here later and see where we are at with the mounting issue. That is the final issue to solve I believe, everything else is looking great.

tarang9211 · 2017-02-14T17:59:46Z

Interesting. Sounds good, let me know your findings. Let's try to get this merged soon!

tarang9211 · 2017-02-15T11:05:30Z

@layershifter Could you help look into this? We're pretty close to a merge

tarang9211 · 2017-02-18T19:08:33Z

@levithomason I suggest we revert the changes we made regarding the Adsense and have it be like what @layershifter did here. That's how everyone seems to be implementing the Adsense with React. Thoughts?

I don't think we faced any issues with that method of implementation.

levithomason · 2017-02-21T16:29:52Z

OK, let's give it a try. Whatever solves the problem is the correct answer :)

tarang9211 · 2017-02-21T16:32:07Z

@levithomason Could you take a look at it? I'm looking at the Checkbox issue(s) currently. Let me know if that's not feasible.

…React into feat/new-component-advertisement

layershifter

@levithomason I've updated typings structure and reverted standart example. LGTM, I think we ready to go 🚤

tarang9211 · 2017-03-03T08:24:50Z

@layershifter Thanks for getting to this! Appreciate the help

levithomason · 2017-03-03T17:02:06Z

Nice work fellas! 🎉

* (new-componenet): added boilerplate code for new advertisement component * (advertisement-component): added conformance test, but failing. * (feat): fixedcode styling issues, 1 test failing * test(Advertisement): add children test * fix(Advertisement): fix typo * fix(Advertisement): fix lint issue * fix(Advertisement): added doc site example for medium, rectangle props * test(Advertisement): added test case for advertisement component * fix(Advertisement): fixed propTypes * fix(Advertisement): fixed displaying text on the ad * fix(Advertisement): fix typos and added props for centered * fix(Advertisement): added tests * fix(Advertisement): added test for unit prop * fix(Advertisement): added line break * fix(Advertisement): fixed code styling issues in src and test folder * fix(Advertisement): modified doc site examples for advertisement * docs(Advertisement): added example for all advertisement types * docs(Advertisement): added doc site examples for advertisement variations * docs(Advertisement): added doc site examples for mobile advertisement types * fix(Advertisement): fixed code styling * fix(Advertisement): fixed doc site code styling and files for advertisement banner * style(Advertisment): fix spacing * fix(Advertisement): fix dots * fix(Advertisement) * style(Advertisement): fix dots * style(Advertisement): fix import order * style(Advertisment): fix spacing * style(Advertisement): fix order * style(Advertisement): fixed files for rectangle advertisement * style(Advertisement): fixed files for square advertisements * style(Advertisement): fixed files for skyscraper advertisements * style(Advertisement): fixed files for leaderboard advertisement * style(Advertisement): fixed files for square advertisement * fix(Advertisement): added large rectangle example to common units * fix(Advertisement): fixed order of example as per SUI website * fix(Advertisement): removed extra item in test variation advertisement * fix(Advertisement): fixed typo * fix(Advertisement): added typings for Advertisement * style(Advertisement): fixed lint error * fix(Advertisement): displaying google adsense examples as standard ads * fix(Advertisement): removed setTimeout, using window.onload instead * style(Adv): multiple fixes * style(Adv): typings fixes * style(Advertisement): fixed code styling * style(Advertisement): removed async attribute from script tag * fix(Advertisement): fixed google ads code * docs(Advertisement): minor grammar correction' * docs(Advertisement): fix lint issue * style(Advertisement): update typings, example

levithomason · 2017-03-13T16:29:41Z

Released in semantic-ui-react@0.67.1.

(new-componenet): added boilerplate code for new advertisement component

6ef91b3

tarang9211 mentioned this pull request Feb 6, 2017

<Advertisement /> Component #185

Closed

(advertisement-component): added conformance test, but failing.

5e34b35

layershifter requested changes Feb 7, 2017

View reviewed changes

layershifter changed the title ~~(new-component): added boilerplate code for new advertisement component~~ feat(Advertisement): add new component Feb 7, 2017

layershifter added the needs author feedback label Feb 7, 2017

(feat): fixedcode styling issues, 1 test failing

050ca88

layershifter added 3 commits February 7, 2017 12:00

test(Advertisement): add children test

5287263

fix(Advertisement): fix typo

9894361

fix(Advertisement): fix lint issue

a443701

tarang9211 added 2 commits February 8, 2017 18:11

fix(Advertisement): added doc site example for medium, rectangle props

1ad75ba

test(Advertisement): added test case for advertisement component

fc903f4

tarang9211 added 3 commits February 9, 2017 01:28

fix(Advertisement): fixed propTypes

aee55c8

fix(Advertisement): fixed displaying text on the ad

a7c2a19

fix(Advertisement): fix typos and added props for centered

990bad3

levithomason added 2 commits February 14, 2017 08:20

docs(Advertisement): minor grammar correction'

595ce10

docs(Advertisement): fix lint issue

59e511d

levithomason approved these changes Feb 14, 2017

View reviewed changes

levithomason added needs engineering and removed ready for review labels Feb 14, 2017

levithomason added needs author feedback and removed needs engineering labels Feb 21, 2017

Alexander Fedyashov added 2 commits March 3, 2017 10:03

Merge branch 'master' of https://github.com/Semantic-Org/Semantic-UI-…

d94c7f9

…React into feat/new-component-advertisement

style(Advertisement): update typings, example

d3ec665

layershifter approved these changes Mar 3, 2017

View reviewed changes

layershifter added ready for review and removed needs author feedback labels Mar 3, 2017

levithomason merged commit d780270 into Semantic-Org:master Mar 3, 2017

layershifter mentioned this pull request Mar 7, 2017

docs(readme): add Advertisement component to list #1421

Merged

		@@ -0,0 +1,40 @@
		import React, { PropTypes } from 'react'
		import cx from 'classnames'

feat(Advertisement): add new component #1292

feat(Advertisement): add new component #1292

Conversation

tarang9211 commented Feb 6, 2017 • edited Loading

codecov-io commented Feb 6, 2017 • edited Loading

Codecov Report

tarang9211 commented Feb 6, 2017 • edited Loading

layershifter left a comment • 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

tarang9211 commented Feb 7, 2017 • edited Loading

layershifter commented Feb 7, 2017

tarang9211 commented Feb 7, 2017

tarang9211 commented Feb 8, 2017

layershifter commented Feb 8, 2017 • edited Loading

tarang9211 commented Feb 8, 2017

tarang9211 commented Feb 8, 2017

layershifter commented Feb 8, 2017

layershifter commented Feb 8, 2017 • edited Loading

Thoughts

tarang9211 commented Feb 8, 2017

jeffcarbs commented Feb 8, 2017

layershifter commented Feb 8, 2017

tarang9211 commented Feb 8, 2017 • edited Loading

tarang9211 commented Feb 14, 2017

tarang9211 commented Feb 14, 2017 via email

levithomason commented Feb 14, 2017

tarang9211 commented Feb 14, 2017 via email

levithomason commented Feb 14, 2017

levithomason commented Feb 14, 2017

tarang9211 commented Feb 14, 2017

tarang9211 commented Feb 15, 2017

tarang9211 commented Feb 18, 2017 • edited Loading

levithomason commented Feb 21, 2017

tarang9211 commented Feb 21, 2017

layershifter left a comment

Choose a reason for hiding this comment

tarang9211 commented Mar 3, 2017

levithomason commented Mar 3, 2017

levithomason commented Mar 13, 2017

tarang9211 commented Feb 6, 2017 •

edited

Loading

codecov-io commented Feb 6, 2017 •

edited

Loading

tarang9211 commented Feb 6, 2017 •

edited

Loading

layershifter left a comment •

edited

Loading

tarang9211 commented Feb 7, 2017 •

edited

Loading

layershifter commented Feb 8, 2017 •

edited

Loading

layershifter commented Feb 8, 2017 •

edited

Loading

tarang9211 commented Feb 8, 2017 •

edited

Loading

tarang9211 commented Feb 18, 2017 •

edited

Loading