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

Site Settings: add date time format sub-page #11259

Merged
merged 28 commits into from
Mar 14, 2017
Merged
Show file tree
Hide file tree
Changes from 25 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
db973be
Add Date and Time Format settings sub-page
Copons Feb 8, 2017
d543766
Add Date and Time Format linked card in general settings
Copons Feb 8, 2017
182ded9
Add DateTimeFormatOptions component
Copons Feb 9, 2017
28ba0cd
Use actual values instead of placeholder formats
Copons Feb 14, 2017
3b20156
Remove internal handleSelect method, replaced with HOC’s one
Copons Feb 14, 2017
ff8e9a6
Move date format to sub-page
Copons Feb 14, 2017
58b4458
Move time format to sub-page
Copons Feb 14, 2017
b66ec33
Remove start of week option from main page
Copons Feb 14, 2017
b2aa6ea
Remove wrong section header label
Copons Feb 14, 2017
0548aa0
Update copy
Copons Feb 16, 2017
227a492
Make settings camelCase and open docs link in a new tab
Copons Feb 17, 2017
aa8bbb8
Move default formats out of the component
Copons Feb 17, 2017
6018d26
Introduce custom format radio button logic
Copons Feb 17, 2017
eebf862
Update custom formats style
Copons Feb 17, 2017
a388e06
Update text colors to a11y standards
Copons Feb 20, 2017
3ed84f7
Abstract setFormat, setCustomFormat handlers
Copons Feb 22, 2017
9842128
Move the current date-time timezone adjustment in its own documented …
Copons Feb 22, 2017
249647d
Add i18n comments
Copons Feb 22, 2017
8c68099
Readability improvements
Copons Feb 23, 2017
4c439b2
Move `getNow()` in an utility file
Copons Feb 23, 2017
289e576
Missing “no-unused-vars” fix left unstaged
Copons Feb 23, 2017
6c30526
Move start of week option into its own component
Copons Feb 28, 2017
8eb467d
Move date format option into its own component
Copons Feb 28, 2017
2c3a1de
Move time format option into its own component
Copons Feb 28, 2017
a3b40b8
Move default formats in an external file and clean up props naming
Copons Feb 28, 2017
a15c407
Rename `now` as `localizedDate` to improve clarity
Copons Mar 7, 2017
a3cf018
Improve code and readability
Copons Mar 7, 2017
7c1edeb
Make `defaultSettings` a module constant
Copons Mar 7, 2017
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
1 change: 1 addition & 0 deletions assets/stylesheets/_components.scss
Original file line number Diff line number Diff line change
Expand Up @@ -318,6 +318,7 @@
@import 'my-sites/site-settings/action-panel/style';
@import 'my-sites/site-settings/custom-content-types/style';
@import 'my-sites/site-settings/comment-display-settings/style';
@import 'my-sites/site-settings/date-time-format/style';
@import 'my-sites/site-settings/delete-site/style';
@import 'my-sites/site-settings/delete-site-options/style';
@import 'my-sites/site-settings/delete-site-warning-dialog/style';
Expand Down
8 changes: 8 additions & 0 deletions client/my-sites/site-settings/controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import SiteSettingsComponent from 'my-sites/site-settings/main';
import sitesFactory from 'lib/sites-list';
import StartOver from './start-over';
import Taxonomies from './taxonomies';
import DateTimeFormat from './date-time-format';
import { setDocumentHeadTitle as setTitle } from 'state/document-head/actions';
import titlecase from 'to-title-case';
import utils from 'lib/site/utils';
Expand Down Expand Up @@ -179,6 +180,13 @@ module.exports = {
);
},

dateTimeFormat( context ) {
renderPage(
context,
<DateTimeFormat />
);
},

legacyRedirects( context, next ) {
const section = context.params.section,
redirectMap = {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
/**
* External dependencies
*/
import React from 'react';
import { localize } from 'i18n-calypso';

/**
* Internal dependencies
*/
import FormFieldset from 'components/forms/form-fieldset';
import FormInput from 'components/forms/form-text-input';
import FormLabel from 'components/forms/form-label';
import FormRadio from 'components/forms/form-radio';
import FormSettingExplanation from 'components/forms/form-setting-explanation';
import { phpToMomentDatetimeFormat } from 'lib/formatting';
import { defaultDateFormats } from './default-formats';

export const DateFormatOption = ( {
dateFormat,
disabled,
isCustom,
now,
setCustomDateFormat,
setDateFormat,
translate,
} ) => (
<FormFieldset>
<FormLabel>
{ translate( 'Date Format' ) }
</FormLabel>
{ defaultDateFormats.map( ( format, index ) =>
<FormLabel key={ index }>
Copy link
Member

Choose a reason for hiding this comment

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

danger, React key as loop index!

<FormRadio
checked={ ! isCustom && format === dateFormat }
disabled={ disabled }
name="date_format"
onChange={ setDateFormat }
value={ format }
/>
<span>{ now.format( phpToMomentDatetimeFormat( format ) ) }</span>
</FormLabel>
) }
<FormLabel className="date-time-format__custom-field">
<FormRadio
checked={ isCustom }
disabled={ disabled }
name="date_format"
onChange={ setCustomDateFormat }
value={ dateFormat }
/>
<span>
{ translate( 'Custom', { comment: 'Custom date/time format field' } ) }
<FormInput
disabled={ disabled }
name="date_format_custom"
onChange={ setCustomDateFormat }
type="text"
value={ dateFormat || '' }
/>
<FormSettingExplanation>
{ isCustom && dateFormat
? translate( 'Preview: %s', {
args: now.format( phpToMomentDatetimeFormat( dateFormat ) ),
comment: 'Date/time format preview',
} )
: ''
Copy link
Member

Choose a reason for hiding this comment

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

I'm confused at the ternary here. Is the goal only to output a string when isCustom && dateFormat? If so, we can just just another &&

{ isCustom && dateFormat && translate(
	'Preview: %s',
	{
		args: ,
		comment: ,
	}
) }

}
</FormSettingExplanation>
</span>
</FormLabel>
</FormFieldset>
);

export default localize( DateFormatOption );
Original file line number Diff line number Diff line change
@@ -0,0 +1,171 @@
/**
* External dependencies
*/
import React, { Component } from 'react';
import { localize } from 'i18n-calypso';
import { capitalize, includes } from 'lodash';

/**
* Internal dependencies
*/
import Button from 'components/button';
import Card from 'components/card';
import SectionHeader from 'components/section-header';
import DateFormatOption from './date-format-option';
import StartOfWeekOption from './start-of-week-option';
import TimeFormatOption from './time-format-option';
import { defaultDateFormats, defaultTimeFormats } from './default-formats';
import { getNow } from './utils';
import wrapSettingsForm from '../wrap-settings-form';

export class DateTimeFormatOptions extends Component {
state = {
customDateFormat: false,
customTimeFormat: false,
isLoadingSettings: true,
};

componentWillReceiveProps( nextProps ) {
const {
fields: {
date_format: dateFormat,
time_format: timeFormat,
},
} = nextProps;

if ( ! this.state.isLoadingSettings || '' === dateFormat || '' === timeFormat ) {
return;
}

this.setState( {
customDateFormat: ! includes( defaultDateFormats, dateFormat ),
customTimeFormat: ! includes( defaultTimeFormats, timeFormat ),
isLoadingSettings: false,
} );
}

setFormat = ( name, defaultFormats ) => event => {
const { value: format } = event.currentTarget;
this.props.updateFields( { [ `${ name }_format` ]: format } );
this.setState( {
[ `custom${ capitalize( name ) }Format` ]: ! includes( defaultFormats, format ),
} );
};

setDateFormat = this.setFormat( 'date', defaultDateFormats );

setTimeFormat = this.setFormat( 'time', defaultTimeFormats );

setCustomFormat = name => event => {
const { value: format } = event.currentTarget;
this.props.updateFields( { [ `${ name }_format` ]: format } );
this.setState( {
[ `custom${ capitalize( name ) }Format` ]: true,
} );
};

setCustomDateFormat = this.setCustomFormat( 'date' );

setCustomTimeFormat = this.setCustomFormat( 'time' );

render() {
const {
fields: {
date_format: dateFormat,
start_of_week: startOfWeek,
time_format: timeFormat,
timezone_string: timezoneString,
},
handleSelect,
handleSubmitForm,
isRequestingSettings,
isSavingSettings,
translate,
} = this.props;

const {
customDateFormat,
customTimeFormat,
} = this.state;

const now = getNow( timezoneString );

return (
<div>
<SectionHeader>
<Button
compact={ true }
onClick={ handleSubmitForm }
primary={ true }
type="submit"
disabled={ isRequestingSettings || isSavingSettings }>
{ isSavingSettings
Copy link
Member

Choose a reason for hiding this comment

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

small nitpick: this confused me here (also a big reason I prefer to keep the > on the next line) because of the indentation. could we move that closing > into alignment with <Button?

<Button
	compact={ true }
	
>
	{ isSavingSettings
		? 
		: 
	}
</Button>

? translate( 'Saving…' )
: translate( 'Save Settings' )
}
</Button>
</SectionHeader>
<Card>
<form>
<DateFormatOption
dateFormat={ dateFormat }
disabled={ isRequestingSettings }
isCustom={ customDateFormat }
now={ now }
setCustomDateFormat={ this.setCustomDateFormat }
setDateFormat={ this.setDateFormat }
/>
<TimeFormatOption
disabled={ isRequestingSettings }
isCustom={ customTimeFormat }
now={ now }
setCustomTimeFormat={ this.setCustomTimeFormat }
setTimeFormat={ this.setTimeFormat }
timeFormat={ timeFormat }
/>
<StartOfWeekOption
disabled={ isRequestingSettings }
onChange={ handleSelect }
startOfWeek={ startOfWeek }
/>
</form>
</Card>
</div>
);
}
}

export default wrapSettingsForm( settings => {
const defaultSettings = {
date_format: '',
start_of_week: 0,
time_format: '',
timezone_string: '',
};
Copy link
Member

Choose a reason for hiding this comment

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

if these are always constant then we might consider moving them into the module-global scope

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. That whole part was basically brought over form-general.jsx (which yes, could use a brush up).


if ( ! settings ) {
return defaultSettings;
}

const formSettings = {
date_format: settings.date_format,
start_of_week: settings.start_of_week,
time_format: settings.time_format,
timezone_string: settings.timezone_string,
};
Copy link
Member

Choose a reason for hiding this comment

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

fun fact! we can use object assignment to provide defaults. I'm not sure if you want the default switch to be "either use all of the defaults or use none of them" but if you want to instead say "use any of the defaults if their corresponding value isn't set" then we can do this:

const settings = {
	...defaultSettings,
	...customSettings,
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The thing here is that I don't need all the settings (there are currently 64 settings, some of them are arrays of objects), so I'd need at least a lodash.pick( ... ) anyway.

Then, the assumption is that, if settings exist, it's already populated server-side.
defaultSettings is pretty much there only for the first uncached load, and that's why it returns early and doesn't let the function proceed to the gmt_offset logic.

So while I'd love to spread here, I'd say it would result in more code (and conditionals) for no real readability improvements.


// handling `gmt_offset` and `timezone_string` values
const gmt_offset = settings.gmt_offset;

if (
! settings.timezone_string &&
typeof gmt_offset === 'string' &&
gmt_offset.length
) {
formSettings.timezone_string = 'UTC' +
( /\-/.test( gmt_offset ) ? '' : '+' ) +
gmt_offset;
}

return formSettings;
} )( localize( DateTimeFormatOptions ) );
12 changes: 12 additions & 0 deletions client/my-sites/site-settings/date-time-format/default-formats.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
export const defaultDateFormats = [
'F j, Y',
'Y-m-d',
'm/d/Y',
'd/m/Y',
];

export const defaultTimeFormats = [
'g:i a',
'g:i A',
'H:i',
];
39 changes: 39 additions & 0 deletions client/my-sites/site-settings/date-time-format/index.jsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
/**
* External dependencies
*/
import React from 'react';
import { connect } from 'react-redux';
import { localize } from 'i18n-calypso';
import page from 'page';

/**
* Internal dependencies
*/
import DateTimeFormatOptions from './date-time-format-options';
import DocumentHead from 'components/data/document-head';
import HeaderCake from 'components/header-cake';
import { getSelectedSite } from 'state/ui/selectors';

export const DateTimeFormat = ( { translate, site } ) => {
const goBack = () => {
page( '/settings/general/' + site.slug );
};
Copy link
Member

Choose a reason for hiding this comment

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

ping @aduth and @ockham for internal use of page() for routing - nothing significant, just noting another use of it

for new code, should we consider creating a new navigation Redux action whose middleware simply calls page() so we can start to phase out the imperative navigation?

Copy link
Contributor

Choose a reason for hiding this comment

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

for new code, should we consider creating a new navigation Redux action whose middleware simply calls page() so we can start to phase out the imperative navigation?

I'd be on board with that 👍

Copy link
Member

Choose a reason for hiding this comment

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

maybe I'll start targeting this as soon as I close up some HTTP layer work…

a simple middleware could handle many of the page() uses methinks


return (
<div className="main main-column date-time-format" role="main">
<DocumentHead title={ translate( 'Manage Date and Time Format' ) } />
<HeaderCake onClick={ goBack }>
<h1>
{ translate( 'Date and Time Format' ) }
</h1>
</HeaderCake>
<DateTimeFormatOptions />
</div>
);
};

const mapStateToProps = state => ( {
site: getSelectedSite( state ),
} );

export default connect( mapStateToProps )( localize( DateTimeFormat ) );
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
/**
* External dependencies
*/
import React from 'react';
import { localize } from 'i18n-calypso';

/**
* Internal dependencies
*/
import FormFieldset from 'components/forms/form-fieldset';
import FormLabel from 'components/forms/form-label';
import FormSelect from 'components/forms/form-select';

export const StartOfWeekOption = ( {
disabled,
moment,
onChange,
startOfWeek,
translate,
} ) =>
<FormFieldset>
<FormLabel>
{ translate( 'Week starts on' ) }
Copy link

Choose a reason for hiding this comment

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

Hi! I've found a possible matching string that has already been translated 88 times:
translate( 'Week Starts On' ) ES Score: 14

Help me improve these suggestions: react with 👎 if the suggestion doesn't make any sense, or with 👍 if it's a particularly good one (even if not implemented).

Copy link

Choose a reason for hiding this comment

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

Hi! I've found a possible matching string that has already been translated 88 times:
translate( 'Week Starts On' ) ES Score: 14

Help me improve these suggestions: react with 👎 if the suggestion doesn't make any sense, or with 👍 if it's a particularly good one (even if not implemented).

</FormLabel>
<FormSelect
disabled={ disabled }
name="start_of_week"
onChange={ onChange }
value={ startOfWeek || 0 }
>
{ moment.weekdays().map( ( day, index ) =>
<option key={ index } value={ index }>
Copy link
Member

@dmsnell dmsnell Mar 7, 2017

Choose a reason for hiding this comment

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

it's no more difficult to just use day as the key here but it's much more reliable and semantic

{ day }
</option>
) }
</FormSelect>
</FormFieldset>;

export default localize( StartOfWeekOption );
10 changes: 10 additions & 0 deletions client/my-sites/site-settings/date-time-format/style.scss
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
.date-time-format__custom-field {
.form-radio {
margin-top: 12px;
}

.form-text-input {
margin: 0 12px;
width: 100px;
}
}
Loading