Splash page #131

manatarms · 2017-06-20T17:06:59Z

This PR adds a new splash page and corresponding tests
Upgrades yarn deps and adds new eslint rules
Fixes eslint errors

jwineman · 2017-06-20T17:08:48Z

@thellimist I told @manatarms to make the other changes around package.json etc.

jwineman · 2017-06-20T17:09:15Z

.eslintrc.json

@@ -0,0 +1,73 @@
+{


Matches www-next

jwineman · 2017-06-20T17:13:50Z

src/selectors/generateChannelLink.js

+ * @param {string} integrationName - Will append channel depending on integration name.
+ */
+
+export function generateChannelLink(pageURL, integrationName) {


Talk to Garrett and see if we can rework the channel values so they can be generated dynamically. I want to avoid hard coding channel values so we don't have to modify code to add new ones.

thellimist · 2017-06-20T17:18:53Z

src/containers/App/App.js

+                : <LayoutRow>{this.props.children}</LayoutRow>}
+              <LayoutRow>
+                <LayoutColumn width={1 / 1}>
+                  <p style={{ textAlign: 'center' }}>


instead of <p> we can use <Text>

There are a few more places where <p> or <a> is used rather than the component. @jwineman is it worth to change every time someone uses non cf-ui elements?

I'm not a 100% sure but I think the Text component might get deprecated. So I didn't want to introduce it as a dep.

<Text> isn't deprecated, please use it here.

Coolios. There's also a bunch of <p> tags on Login, should I switch those out too

We probably did that to inject those styles, if you can inject those styles without <p> great, if its a lot of work don't worry about it.

thellimist · 2017-06-20T17:24:32Z

src/routes.js

@@ -9,6 +9,7 @@ import App from './containers/App/App';
 import DNSManagementPage
  from './containers/DNSManagementPage/DNSManagementPage';
 import Login from './containers/LoginPage/LoginPage';
+import Splash from './containers/SplashPage/SplashPage';


Shouldn't this be import SplashPage and import LoginPage?

It should totally be that.

thellimist · 2017-06-20T17:30:49Z

src/selectors/generateChannelLink.js

+      break;
+    default:
+      channelName = '';
+  }


generateChannelLink("url.com", "somethingisnotright") returns "url.com?channel="

It would be better to not generate the channel if it's going to be ''. But I think it may be better to put something there. It may allows us figure if something is wrong or if someone else has built there own custom integration using this code. Both are fine information I guess.

Are you thinking of an error param if someone tries to append a channel directly?

John mentioned allowing to make this dynamic which makes a lot of sense. If we the code stays like this for some reason, I'd suggest something like "Cloudflare Plugin Frontend". Talk to Garrett for this

thellimist · 2017-06-20T17:33:12Z

src/test/containers/SplashPageTest.js

+  'component.benefitsFeature.security.description': 'Cloudflare offers protections against vulnerabilities including DDoS protection.',
+  'component.benefitsFeature.insights.title': 'Insights',
+  'component.benefitsFeature.insights.description': 'Monitor bandwidth saved, threats blocked, and more with built-in analytics.'
+};


~~If we need to change the language, the tests will fail adding additional complexity. I'd suggest to import en.js rather than coping the translations~~

Tests should mock translations, not use them directly. We aren't testing the specific translations, we're testing that given the correct keys it uses their values.

@manatarms I think this and svg are the only things left.

Yup spoke to Eric, he'll give me the svgs. I'll merge after that.

@manatarms from my experience exporting svg's from Sketch worked the best. When I got them exported from Adobe Illustrator the format was not transferable into React component's. You can look in cf-ux/cf-component-signup/Security how we did the migration.

I'm not sure what frontend best practices for tests are but I think this could cause confusion later on. It may be better to make it clear that these are mock values. Ignore me if I think incorrectly

I agree mock values are better but this is a test so its probably safe to assume that even if the data looks real it is mocked.

thellimist · 2017-06-20T17:34:00Z

src/test/selectors/generateChannelLinkTest.js

+    expect(generateChannelLink('htttp://www.example.com', 'cpanel')).toBe(
+      'htttp://www.example.com?channel=Integration:%20CPanel'
+    );
+  });


could you add magento and default cases here?

I'll do it once I change talk with Garrett about the channel names

We only need to test

generateChannelLink('htttp://www.example.com', 'something')
generateChannelLink('htttp://www.example.com', '')
generateChannelLink('htttp://www.example.com')

Sure. Some of these tests were from the switch case. Will change

Actually the tests make sense because it tests for params and also we're returning specific things based on the integration. I think removing the magento and someNewChannel test makes sense but the rest should stay.

sejoker · 2017-06-22T09:17:07Z

I recommend using svg format for icons instead of png.

jwineman · 2017-06-23T22:28:11Z

@sejoker Good call. @manatarms worked with Eric to get those assets in SVG, he'll update the PR next week.

manatarms · 2017-06-27T21:05:23Z

Okay this looks good to go! Would appreciate a final look.

thellimist · 2017-06-28T10:33:38Z

src/containers/SplashPage/SplashPage.js

@@ -59,16 +59,20 @@ class SplashPage extends Component {
  render() {
    const { config } = this.props;

+    const integrationName = getConfigValue(config, 'integrationName');
+    const integrationNameCapital =
+      integrationName.charAt(0).toUpperCase() + integrationName.slice(1);


It's a small detail but this will produce Wordpress rather than WordPress. Check if Garrett is OK with it.

jwineman requested review from thellimist and jwineman June 20, 2017 17:08

jwineman reviewed Jun 20, 2017

View reviewed changes

.eslintrc.json

@@ -0,0 +1,73 @@

{

Copy link

Contributor

jwineman Jun 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Matches www-next

thellimist reacted with thumbs up emoji

jwineman reviewed Jun 20, 2017

View reviewed changes

jwineman approved these changes Jun 20, 2017

View reviewed changes

thellimist reviewed Jun 20, 2017

View reviewed changes

manatarms added 9 commits June 27, 2017 10:41

Adde new splash page

262110a

Yarn upgrade and new eslint rules

dfa5621

Fixed eslint errors

07693c2

Changed route names to better names

9c0d829

Changed the channel values and added more tests for channel function

1d3b770

Added integration specific heading

2435ab7

Updated tests for channel generator

60d061b

Fix conflict

21b41b3

Changed to svgs

2e16701

manatarms force-pushed the splash-page branch from 57ae308 to 2e16701 Compare June 27, 2017 21:03

Updated tests

321aa21

jwineman approved these changes Jun 27, 2017

View reviewed changes

sejoker self-requested a review June 28, 2017 10:28

sejoker approved these changes Jun 28, 2017

View reviewed changes

thellimist reviewed Jun 28, 2017

View reviewed changes

thellimist approved these changes Jun 28, 2017

View reviewed changes

Merge branch 'master' into splash-page

63f740d

manatarms merged commit b4b4c72 into master Jun 28, 2017

manatarms deleted the splash-page branch June 28, 2017 16:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Splash page #131

Splash page #131

manatarms commented Jun 20, 2017

jwineman commented Jun 20, 2017

jwineman Jun 20, 2017

jwineman Jun 20, 2017

thellimist Jun 20, 2017

thellimist Jun 20, 2017 •

edited

Loading

manatarms Jun 20, 2017

jwineman Jun 20, 2017

manatarms Jun 20, 2017 •

edited

Loading

jwineman Jun 20, 2017

thellimist Jun 20, 2017

manatarms Jun 20, 2017

thellimist Jun 20, 2017

manatarms Jun 20, 2017

thellimist Jun 20, 2017 •

edited

Loading

thellimist Jun 20, 2017 •

edited

Loading

jwineman Jun 20, 2017

thellimist Jun 23, 2017

manatarms Jun 23, 2017

sejoker Jun 26, 2017

thellimist Jun 28, 2017 •

edited

Loading

jwineman Jun 28, 2017

thellimist Jun 20, 2017

manatarms Jun 20, 2017

thellimist Jun 20, 2017

manatarms Jun 22, 2017

manatarms Jun 22, 2017

sejoker commented Jun 22, 2017

jwineman commented Jun 23, 2017

manatarms commented Jun 27, 2017

thellimist Jun 28, 2017

Splash page #131

Splash page #131

Conversation

manatarms commented Jun 20, 2017

jwineman commented Jun 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thellimist Jun 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manatarms Jun 20, 2017 • 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

thellimist Jun 20, 2017 • edited Loading

Choose a reason for hiding this comment

thellimist Jun 20, 2017 • 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

thellimist Jun 28, 2017 • 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

sejoker commented Jun 22, 2017

jwineman commented Jun 23, 2017

manatarms commented Jun 27, 2017

Choose a reason for hiding this comment

thellimist Jun 20, 2017 •

edited

Loading

manatarms Jun 20, 2017 •

edited

Loading

thellimist Jun 20, 2017 •

edited

Loading

thellimist Jun 20, 2017 •

edited

Loading

thellimist Jun 28, 2017 •

edited

Loading