Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Notices: Redux everything! #1393

Closed
wants to merge 34 commits into from
Closed
Show file tree
Hide file tree
Changes from 31 commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
6a4462f
Plugin-browser-list: refactored to ES6
johnHackworth Dec 7, 2015
99d6d57
Plugins-Browser-list: Apply section header
johnHackworth Dec 7, 2015
abe9eb8
Plugins-browser: Use Card for plugin browser lists
johnHackworth Dec 7, 2015
2957c66
Plugins-browser: Fix 'see all' button padding
johnHackworth Dec 7, 2015
3c571fe
Me: Fixes logic for gated security checkup nav tab
ebinnion Dec 7, 2015
0320323
Section header: Vertical align label
johnHackworth Dec 9, 2015
969fd8b
Framework: Check for gridicon sizes on commit
johnHackworth Dec 9, 2015
718fde2
Gridicons: adds 'nonStandard' prop so the precommit checker can skip …
johnHackworth Dec 10, 2015
e8ae82a
Gridicons: Support for multiline & several gridicons in the same file
johnHackworth Dec 10, 2015
9edc979
Gridicons: change the nonStandard property name to reflect it only ma…
johnHackworth Dec 10, 2015
dc75c32
Section Header: move to a standard font size of 11px
enejb Dec 10, 2015
3a6e337
Merge pull request #751 from Automattic/add/sites-redux
aduth Dec 11, 2015
303ae86
Merge pull request #1353 from Automattic/fix/me-checkup-section-nav
ebinnion Dec 11, 2015
ab12dc4
Merge pull request #1412 from Automattic/add/pre-commit-gridicons-check
johnHackworth Dec 11, 2015
a233628
Merge pull request #1321 from Automattic/add/plugin-browser-section-h…
johnHackworth Dec 11, 2015
6b8454c
Notices: connect Layouts to global redux store
artpi Dec 8, 2015
9f2932c
Notices: Add global-notices component
artpi Dec 8, 2015
1c90e2d
Notices: Inject global-notices component into layouts
artpi Dec 8, 2015
fac06c2
Notices: Add proper reducers
artpi Dec 8, 2015
7c91bf3
Notices: move global state to singleton
artpi Dec 9, 2015
b29b6ec
Notices: add actions for notices
artpi Dec 9, 2015
c91799a
Notices: add global notices handler
artpi Dec 9, 2015
d6e5798
Notices: Use global notices handler to display notices
artpi Dec 9, 2015
8d1dbde
Notices: Remove helper
artpi Dec 10, 2015
b81249c
Notices: Connect notification settings component to react-redux
artpi Dec 10, 2015
faa3e18
Revert "Notices: move global state to singleton"
artpi Dec 10, 2015
f107d47
Notices: Proper formatting of tabs
artpi Dec 10, 2015
113f920
Notices: remove extra chars from actionCreator
artpi Dec 10, 2015
f07e648
Notices: rename reduxStore to store
artpi Dec 10, 2015
e54af03
Notices: clean up GlobalNotices minor problems
artpi Dec 10, 2015
8f3d3b3
Notices: Clean up connected components in boot
artpi Dec 10, 2015
5b50b05
Notices: Add dismissing of notice
artpi Dec 11, 2015
5da94cb
Notices: Add option duration
artpi Dec 11, 2015
a2f7f79
Notices: Move notices controller to actionCreators
artpi Dec 11, 2015
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 51 additions & 0 deletions bin/gridiconFormatChecker
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
#!/usr/bin/env node

var fs = require( 'fs' );
var validIconSizes = [ 12, 18, 24, 36, 48, 54, 72 ];

var filename = process.argv[ 2 ];

fs.readFile( filename, 'utf8', function ( err, data ) {
var result = '',
splittedCode,
lineNumber = 1;
if ( err ) {
console.log(err);
process.exit( 1 );
}
data = data.toLowerCase();
splittedCode = data.split( '<gridicon' );

if ( splittedCode.length > 1 ) {
// There are gridicon instances in this file.
splittedCode.forEach( function( chunk ) {
var gridiconAttrs, isNonStandard, size;
if( chunk ) {
// we discard all the code after the tag closing... we are only interested in the props of the gridicon.
gridiconAttrs = chunk.split( '>' )[ 0 ];
isNonStandard = gridiconAttrs.indexOf( 'nonstandardsize' ) >= 0;
if ( gridiconAttrs.indexOf( 'size={' ) >= 0 ) {
size = gridiconAttrs.split( 'size={' )[ 1 ].split( '}' )[ 0 ];
if ( !isNaN( size ) ) {
// We only can check if the size is standard if it is a number. If not (variables), we have no way of knowing if it's fine or not
if( !isNonStandard && validIconSizes.indexOf( +size ) < 0 ) {
result += '\033[31mNon-standard gridicon size ( ' + size + 'px ) detected in ' + filename + ' line ' + lineNumber + '\n';
}
if( isNonStandard && validIconSizes.indexOf( +size ) >= 0 ) {
result += '\033[33mStandard size gridicon ( ' + size + 'px ) marked as non-standard... are you sure that is ok? in ' + filename + ' line ' + lineNumber + '\n';
}
}
}
lineNumber += chunk.split('\n').length - 1;
}
} );
}

if ( result !== '' ) {
console.error( result );
console.log( '\033[m=== Valid gridiconsizes are ' + validIconSizes.join( 'px, ' ) + 'px ===\n' );
process.exit( 1 );
} else {
process.exit( 0 );
}
} );
9 changes: 9 additions & 0 deletions bin/pre-commit
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,15 @@ done

echo "\neslint validation complete\n"

for file in ${files}; do
./bin/gridiconFormatChecker ${file}
if [ $? -ne 0 ]; then
echo "\033[31mGridicon Format Check Failed: \033[0m${file}\n"
pass=false
fi
done


if ! $pass; then
echo "\033[41mCOMMIT FAILED:\033[0m Your commit contains files that should pass validation tests but do not. Please fix the errors and try again.\n"
exit 1
Expand Down
25 changes: 14 additions & 11 deletions client/boot/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ var React = require( 'react' ),
page = require( 'page' ),
url = require( 'url' ),
qs = require( 'querystring' ),
ReduxProvider = require( 'react-redux' ).Provider,
injectTapEventPlugin = require( 'react-tap-event-plugin' );

/**
Expand Down Expand Up @@ -78,9 +79,7 @@ function init() {
} );
}

function setUpContext( layout ) {
var reduxStore = createReduxStore();

function setUpContext( layout, reduxStore ) {
// Pass the layout so that it is available to all page handlers
// and add query and hash objects onto context object
page( '*', function( context, next ) {
Expand Down Expand Up @@ -138,7 +137,7 @@ function loadDevModulesAndBoot() {
}

function boot() {
var layoutSection, layout, validSections = [];
var layoutSection, layout, layoutComponentCreator, reduxStore, validSections = [];

init();

Expand All @@ -153,6 +152,7 @@ function boot() {
} );

translatorJumpstart.init();
reduxStore = createReduxStore();

if ( user.get() ) {
// When logged in the analytics module requires user and superProps objects
Expand All @@ -161,13 +161,14 @@ function boot() {

// Create layout instance with current user prop
Layout = require( 'layout' );
layout = React.render( React.createElement( Layout, {

Copy link
Contributor

Choose a reason for hiding this comment

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

I must have squashed it into non-existence in #338, but in some of my earlier attempts to introduce a top-level provider, I used it as an opportunity to reduce some of the repetition here:

diff --git a/client/boot/index.js b/client/boot/index.js
index 99df3f4..ef7428d 100644
--- a/client/boot/index.js
+++ b/client/boot/index.js
@@ -163,13 +163,13 @@ function boot() {

                // Create layout instance with current user prop
                Layout = require( 'layout' );
-               layout = React.render( React.createElement( Layout, {
+               layout = React.createElement( Layout, {
                        user: user,
                        sites: sites,
                        focus: layoutFocus,
                        nuxWelcome: nuxWelcome,
                        translatorInvitation: translatorInvitation
-               } ), document.getElementById( 'wpcom' ) );
+               } );
        } else {
                analytics.setSuperProps( superProps );

@@ -179,12 +179,14 @@ function boot() {
                        LoggedOutLayout = require( 'layout/logged-out' );
                }

-               layout = React.render(
-                       React.createElement( LoggedOutLayout ),
-                       document.getElementById( 'wpcom' )
-               );
+               layout = React.createElement( LoggedOutLayout );
        }

+       layout = React.render(
+               React.createElement( ReduxProvider, { store: reduxStore }, () => layout ),
+               document.getElementById( 'wpcom' )
+       );
+
        debug( 'Main layout rendered.' );

        // If `?sb` or `?sp` are present on the path set the focus of layout

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will get around to that once we decide what to do with global store (do we want singleton?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pushing right now :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

layoutComponentCreator = () => React.createElement( Layout, {
user: user,
sites: sites,
focus: layoutFocus,
nuxWelcome: nuxWelcome,
translatorInvitation: translatorInvitation
} ), document.getElementById( 'wpcom' ) );
} );
} else {
analytics.setSuperProps( superProps );

Expand All @@ -177,12 +178,14 @@ function boot() {
LoggedOutLayout = require( 'layout/logged-out' );
}

layout = React.render(
React.createElement( LoggedOutLayout ),
document.getElementById( 'wpcom' )
);
layoutComponentCreator = () => React.createElement( LoggedOutLayout );
}

layout = React.render(
React.createElement( ReduxProvider, { store: reduxStore }, layoutComponentCreator ),
document.getElementById( 'wpcom' )
);

debug( 'Main layout rendered.' );

// If `?sb` or `?sp` are present on the path set the focus of layout
Expand All @@ -193,7 +196,7 @@ function boot() {
window.history.replaceState( null, document.title, window.location.pathname );
}

setUpContext( layout );
setUpContext( layout, reduxStore );

page( '*', require( 'lib/route/normalize' ) );

Expand Down
2 changes: 2 additions & 0 deletions client/components/overlay/overlay.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ var React = require( 'react/addons' ),
* Internal dependencies
*/
var Toolbar = require( './toolbar' ),
GlobalNotices = require( 'notices/global-notices' ),
NoticesList = require( 'notices/notices-list' ),
notices = require( 'notices' ),
page = require( 'page' ),
Expand Down Expand Up @@ -101,6 +102,7 @@ module.exports = React.createClass({

<div className="wp-content" ref="overlayInnerContent">
<NoticesList id="overlay-notices" notices={ notices.list }/>
<GlobalNotices id="overlay-notices" />
{ this.props.children }
</div>
</section>
Expand Down
6 changes: 4 additions & 2 deletions client/components/section-header/style.scss
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,13 @@
}

.section-header__label {
display: flex;
align-items: center;
flex-grow: 1;
line-height: 28px;
position: relative;


&:before {
@include long-content-fade( $color : $white );
}
Expand All @@ -33,13 +36,12 @@
.section-header__label,
.section-header__button {
color: $gray;
font-size: 12px;
font-size: 11px;
text-transform: uppercase;
}

.section-header__button {
background: none;
float: let;
margin-right: 8px;
padding: 2px 8px;

Expand Down
2 changes: 2 additions & 0 deletions client/layout/index.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ var React = require( 'react' ),
*/
var Masterbar = require( './masterbar' ),
observe = require( 'lib/mixins/data-observe' ),
GlobalNotices = require( 'notices/global-notices' ),
NoticesList = require( 'notices/notices-list' ),
notices = require( 'notices' ),
translator = require( 'lib/translator-jumpstart' ),
Expand Down Expand Up @@ -111,6 +112,7 @@ module.exports = React.createClass( {
<InviteMessage sites={ this.props.sites }/>
<EmailVerificationNotice user={ this.props.user } />
<NoticesList id="notices" notices={ notices.list } forcePinned={ 'post' === this.state.section } />
<GlobalNotices id="notices" />
<TranslatorInvitation isVisible={ showInvitation } />
<div id="primary" className="wp-primary wp-section" />
<div id="secondary" className="wp-secondary" />
Expand Down
2 changes: 2 additions & 0 deletions client/layout/logged-out.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ var React = require( 'react' ),
*/
var Masterbar = require( './masterbar' ),
NoticesList = require( 'notices/notices-list' ),
GlobalNotices = require( 'notices/global-notices' ),
notices = require( 'notices' );

module.exports = React.createClass( {
Expand All @@ -32,6 +33,7 @@ module.exports = React.createClass( {
<Masterbar />
<div id="content" className="wp-content">
<NoticesList id="notices" notices={ notices.list } />
<GlobalNotices id="notices" />
Copy link
Member

Choose a reason for hiding this comment

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

Can those two components be both rendered at the same time? If that happens, won’t the two HTML elements have the same id?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point!
GlobalNotices is very rudimentary for now, it copies most of NoticesList, but needs a better design.

Copy link
Member

Choose a reason for hiding this comment

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

@artpi see #1415.

<div id="primary" className="wp-primary wp-section" />
<div id="secondary" className="wp-secondary" />
</div>
Expand Down
21 changes: 12 additions & 9 deletions client/me/controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import purchasesController from './purchases/controller';
import userFactory from 'lib/user';
import userSettings from 'lib/user-settings';
import titleActions from 'lib/screen-title/actions';
import { Provider } from 'react-redux';

const ANALYTICS_PAGE_TITLE = 'Me',
devices = devicesFactory(),
Expand Down Expand Up @@ -192,15 +193,17 @@ export default {
analytics.pageView.record( basePath, ANALYTICS_PAGE_TITLE + ' > Notifications' );

React.render(
React.createElement( NotificationsComponent,
{
user: user,
userSettings: userSettings,
blogs: sites,
devices: devices,
path: context.path
}
),
React.createElement( Provider, { store: context.store }, () => {
return React.createElement( NotificationsComponent,
{
user: user,
userSettings: userSettings,
blogs: sites,
devices: devices,
path: context.path
}
)
} ),
document.getElementById( 'primary' )
);
},
Expand Down
22 changes: 18 additions & 4 deletions client/me/notification-settings/index.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import React from 'react';
* Internal dependencies
*/
import observe from 'lib/mixins/data-observe';
import notices from 'notices';
import { success, error } from 'state/notices/actions';
import Main from 'components/main';
import ReauthRequired from 'me/reauth-required';
import twoStepAuthorization from 'lib/two-step-authorization';
Expand All @@ -16,8 +16,9 @@ import Navigation from './navigation';
import BlogsSettings from './blogs-settings';
import store from 'lib/notification-settings-store';
import { fetchSettings, toggle, saveSettings } from 'lib/notification-settings-store/actions';
import { connect } from 'react-redux';

export default React.createClass( {
const NotificationSettings = React.createClass( {
displayName: 'NotificationSettings',

mixins: [ observe( 'sites', 'devices' ) ],
Expand All @@ -43,11 +44,11 @@ export default React.createClass( {
const state = store.getStateFor( 'blogs' );

if ( state.error ) {
notices.error( this.translate( 'There was a problem saving your changes. Please, try again.' ) );
this.props.errorNotice( this.translate( 'There was a problem saving your changes. Please, try again.' ) );
}

if ( state.status === 'success' ) {
notices.success( this.translate( 'Settings saved successfully!' ) );
this.props.successNotice( this.translate( 'Settings saved successfully!' ) );
}

this.setState( state );
Expand All @@ -74,3 +75,16 @@ export default React.createClass( {
}
} );

export default connect(
() => {
return {}
},
( dispatch ) => { return {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the place I would like to have some helper, because implementing this action creators in every place that uses notices would be... verbose.

successNotice: ( text, options ) => {
dispatch( success( text, options ) );
},
errorNotice: ( text, options ) => {
dispatch( error( text, options ) );
}
} }
)( NotificationSettings );
49 changes: 26 additions & 23 deletions client/me/security-section-nav/index.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -18,27 +18,30 @@ module.exports = React.createClass( {
path: React.PropTypes.string.isRequired
},

getDefaultProps: function() {
return {
tabs: [
{
title: i18n.translate( 'Password', { textOnly: true } ),
path: '/me/security',
},
{
title: i18n.translate( 'Two-Step Authentication', { textOnly: true } ),
path: '/me/security/two-step',
},
{
title: i18n.translate( 'Connected Applications', { textOnly: true } ),
path: '/me/security/connected-applications',
},
config.isEnabled( 'me/security/checkup' ) ? {
title: i18n.translate( 'Checkup', { textOnly: true } ),
path: '/me/security/checkup',
} : false
]
};
getNavtabs: function() {
Copy link
Member

Choose a reason for hiding this comment

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

it seems you picked up some unrelated changes?

var tabs = [
{
title: i18n.translate( 'Password', { textOnly: true } ),
path: '/me/security',
},
{
title: i18n.translate( 'Two-Step Authentication', { textOnly: true } ),
path: '/me/security/two-step',
},
{
title: i18n.translate( 'Connected Applications', { textOnly: true } ),
path: '/me/security/connected-applications',
}
];

if ( config.isEnabled( 'me/security/checkup' ) ) {
tabs.push( {
title: i18n.translate( 'Checkup', { textOnly: true } ),
path: '/me/security/checkup',
} );
}

return tabs;
},

getFilteredPath: function() {
Expand All @@ -48,7 +51,7 @@ module.exports = React.createClass( {

getSelectedText: function() {
var text = '',
found = find( this.props.tabs, function( tab ) {
found = find( this.getNavtabs(), function( tab ) {
return this.getFilteredPath() === tab.path;
}, this );

Expand All @@ -67,7 +70,7 @@ module.exports = React.createClass( {
return (
<SectionNav selectedText={ this.getSelectedText() }>
<NavTabs>
{ this.props.tabs.map( function( tab ) {
{ this.getNavtabs().map( function( tab ) {
return (
<NavItem
key={ tab.path }
Expand Down
Loading