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

Copons · 2017-02-08T16:38:24Z

See relevant discussion at #10441 (specifically this comment and the following).

Add a General Settings sub-page for Date Format, Time Format, Week Starts On, and possibly even Timezone.
The actual code handling the settings is being developed in #10400, and will be ported over here after its merge.

Screenshots

GIF

matticbot · 2017-02-08T16:38:26Z

Test live: https://calypso.live/?branch=add/site-settings/date-format-sub-page

a8ci18n · 2017-02-08T16:42:32Z

client/my-sites/site-settings/date-time-format/index.jsx

+
+	return (
+		<div className="main main-column date-time-format" role="main">
+			<DocumentHead title={ translate( 'Manage Date and Time Format Configurations' ) } />


Hi! I've found a possible matching string that has already been translated 52 times:
translate( 'Date and time format:' ) ES Score: 13.14
See 1 additional suggestion in the PR translation status page

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).

Hrm, this comment should have been for the string below, line 26. (different case)

@yoavf I guess you're here for the wrong comment, but please don't waste your time on this PR already, since it's still very WIP.
I'll likely move things around a bit in the next commits, and I'm sure when I'll get some design feedback the translation strings will change too.

Maybe it could make sense to limit the i18n bot to needs review PRs only?
(I'd say to "needs i18n review" only, but I know us devs, we always forget about that label 🙂 )

Makes sense, but I still find it useful to audit every pr for now, to make sure everything works as expected. Once we're more confident we'll probably activate on "needs review".

Ah yup, I meant that, not "ready to merge". :)

a8ci18n · 2017-02-08T16:42:32Z

client/my-sites/site-settings/form-general.jsx

+				href={ `/settings/date-time-format/${ site.slug }` }
+			>
+				<h2 className="site-settings__date-time-format-title">
+					{ translate( 'Date and time format configuration' ) }


Hi! I've found a possible matching string that has already been translated 52 times:
translate( 'Date and time format:' ) ES Score: 6.74
See 1 additional suggestion in the PR translation status page

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).

kristastevens · 2017-02-14T17:58:54Z

Hello! Here are a few items that need love, based on copy review -- thanks for the ping!

Timezone should be two words. It appears twice -- Once in the subtitle, "Site Timezone" again in the microcopy, "Choose a city in your timezone." S/b time zone.
Date and Time Format Configuration > Both format and configuration aren't needed here. Suggest: "Date and Time Format"
Week Starts On Monday" > The caps can be removed: "Week starts on Monday"
"You can also modify the interface language in your profile." A little more personal to say, "You can also modify your interface's language in your profile."
"Documentation on date and time formatting" > I'd make it more friendly by saying, "Learn more about date and time formatting."

Copons · 2017-02-14T18:08:31Z

Thanks @kristastevens!
I'll see to update the copy on the date/time/week format parts and pass along the suggestion regarding the time zone to the team working on it! :)

(Then cue to i18n wrath for so many new translation strings 😄 )

See: #11259 (comment)

a8ci18n · 2017-02-16T17:06:52Z

client/my-sites/site-settings/form-general.jsx

+				<div className="site-settings__date-time-format-info">
+					{ date_format ? today.format( phpToMomentDatetimeFormat( date_format ) ) : '' } &bull;&nbsp;
+					{ time_format ? today.format( phpToMomentDatetimeFormat( time_format ) ) : '' } &bull;&nbsp;
+					{ translate( 'Week starts on' ) } { start_of_week ? daysOfWeek[ start_of_week ] : daysOfWeek[ 0 ] }


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

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).

a8ci18n · 2017-02-16T17:06:52Z

client/my-sites/site-settings/date-time-format/date-time-format-options.jsx

+					</span>
+					<FormSettingExplanation>
+						<ExternalLink href="https://codex.wordpress.org/Formatting_Date_and_Time" icon>
+							{ translate( 'Learn more about date and time formatting.' ) }


Hi! I've found a possible matching string that has already been translated 5 times:
translate( 'Documentation on date and time formatting.' ) ES Score: 6.55

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).

Copons · 2017-02-16T17:12:47Z

@yoavf about the latest commit, please refer to #11259 (comment) for the reason.
I have no strong opinions regarding capitalization, so it's pretty much a fight between better copy vs. less translation strings. :)

yoavf · 2017-02-16T17:58:18Z

@Copons sure, better copy is obviously important. I have some ideas about processes we could set up to copy all existing translations when a string with a different case is added (we already do this right now when we change a string - but it doesn't happen if the changed string also exists elsewhere).

I'll take a look at some of the changes here: when possible (=not a core string), I'll apply the changes to the other instances too, so we don't have duplication.

Edit: see (internal) r151058-wpcom for an example.

roundhill · 2017-02-16T21:01:33Z

client/my-sites/site-settings/date-time-format/date-time-format-options.jsx

+export class DateTimeFormatOptions extends Component {
+	dateFormatOption() {
+		const {
+			fields: { date_format, timezone_string },


What if you destructured these to camelCase names?

{ date_format: dateFormat, timezone_string: timezoneString }

roundhill · 2017-02-16T21:24:31Z

client/my-sites/site-settings/date-time-format/date-time-format-options.jsx

+						</span>
+					</span>
+					<FormSettingExplanation>
+						<ExternalLink href="https://codex.wordpress.org/Formatting_Date_and_Time" icon>


Can ExternalLink open in a new tab/window? I think that'd be good for this link.

a8ci18n · 2017-02-17T16:11:13Z

client/my-sites/site-settings/date-time-format/date-time-format-options.jsx

+						/>
+						<FormSettingExplanation>
+							{ isCustomFormat && dateFormat
+								? translate( 'Preview: %s', {


Hi! I've found a possible matching string that has already been translated 67 times:
translate( 'Preview %s' ) ES Score: 18

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).

davewhitley · 2017-02-17T22:06:14Z

client/my-sites/site-settings/style.scss

-	.site-settings__date-time-format-custom-preview {
-		white-space: nowrap;
+	.site-settings__date-time-format-info {
+		color: $gray;


Change this to $gray-text-min so that it's dark enough!

Done!
I've also replaced $gray-dark with $gray-text as, while they are the same color, it makes more sense to me to use -text for a text color.

davewhitley · 2017-02-17T22:07:04Z

Other than my one comment, I think the design is good to go.

a8ci18n · 2017-03-07T11:07:28Z

client/my-sites/site-settings/form-general.jsx

+				<div className="site-settings__date-time-format-info">
+					{ dateFormat ? now.format( phpToMomentDatetimeFormat( dateFormat ) ) : '' } &bull;&nbsp;
+					{ timeFormat ? now.format( phpToMomentDatetimeFormat( timeFormat ) ) : '' } &bull;&nbsp;
+					{ translate( 'Week starts on' ) } { startOfWeek


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).

Copons · 2017-03-07T11:34:27Z

@dmsnell I've renamed all those nows with localizedDate for consistency with an already established naming convention (see:

wp-calypso/client/components/post-schedule/utils.js

Line 39 in 51a61be

const getLocalizedDate = ( date, tz, gmt ) => {

).

Just FYI, I didn't reuse that function because I would have needed an even more complicated function to handle timezone strings and UTC offset hours-minutes conversions.

a8ci18n · 2017-03-07T11:35:30Z

client/my-sites/site-settings/date-time-format/time-format-option.jsx

+				/>
+				<FormSettingExplanation>
+					{ isCustom && timeFormat
+						? translate( 'Preview: %s', {


Hi! I've found a possible matching string that has already been translated 67 times:
translate( 'Preview %s' ) ES Score: 18

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).

a8ci18n · 2017-03-07T11:35:30Z

client/my-sites/site-settings/form-general.jsx

+						? localizedDate.format( phpToMomentDatetimeFormat( timeFormat ) )
+						: ''
+					} &bull;&nbsp;
+					{ translate( 'Week starts on' ) } { startOfWeek


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).

dmsnell · 2017-03-07T18:30:45Z

client/my-sites/site-settings/date-time-format/date-format-option.jsx

+			{ translate( 'Date Format' ) }
+		</FormLabel>
+		{ defaultDateFormats.map( ( format, index ) =>
+			<FormLabel key={ index }>


danger, React key as loop index!

dmsnell · 2017-03-07T18:32:26Z

client/my-sites/site-settings/date-time-format/date-format-option.jsx

+							args: localizedDate.format( phpToMomentDatetimeFormat( dateFormat ) ),
+							comment: 'Date/time format preview',
+						} )
+						: ''


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: …, } ) }

dmsnell · 2017-03-07T18:34:20Z

client/my-sites/site-settings/date-time-format/date-time-format-options.jsx

+						primary={ true }
+						type="submit"
+						disabled={ isRequestingSettings || isSavingSettings }>
+							{ isSavingSettings


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>

dmsnell · 2017-03-07T18:35:56Z

client/my-sites/site-settings/date-time-format/date-time-format-options.jsx

+		start_of_week: settings.start_of_week,
+		time_format: settings.time_format,
+		timezone_string: settings.timezone_string,
+	};


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, };

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.

dmsnell · 2017-03-07T18:36:51Z

client/my-sites/site-settings/date-time-format/start-of-week-option.jsx

+		value={ startOfWeek || 0 }
+	>
+		{ moment.weekdays().map( ( day, index ) =>
+			<option key={ index } value={ index }>


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

dmsnell · 2017-03-07T18:37:26Z

client/my-sites/site-settings/date-time-format/time-format-option.jsx

+			{ translate( 'Time Format' ) }
+		</FormLabel>
+		{ defaultTimeFormats.map( ( format, index ) =>
+			<FormLabel key={ index }>


danger, React key as loop index!

dmsnell · 2017-03-07T18:37:41Z

client/my-sites/site-settings/date-time-format/time-format-option.jsx

+							args: localizedDate.format( phpToMomentDatetimeFormat( timeFormat ) ),
+							comment: 'Date/time format preview'
+						} )
+						: ''


see my note above on the ternary

dmsnell · 2017-03-07T18:38:09Z

client/my-sites/site-settings/form-general.jsx

+					{ dateFormat
+						? localizedDate.format( phpToMomentDatetimeFormat( dateFormat ) )
+						: ''
+					} &bull;&nbsp;


I'm calling your bull on this one…

dmsnell · 2017-03-07T18:38:39Z

client/my-sites/site-settings/form-general.jsx

+					} &bull;&nbsp;
+					{ timeFormat
+						? localizedDate.format( phpToMomentDatetimeFormat( timeFormat ) )
+						: ''


hrm…was this spitting out false as a string without the ternary…?

dmsnell · 2017-03-07T18:40:30Z

@Copons really coming together with this. I'm a big fan of the idea to follow-up with the token input for date fields - that's so much better of a UI than needing to know date format identifiers.

- Remove numeric `key={ index }`. - Remove unnecessary ternaries. - Use `lodash.pick()` to pick site settings. - Fix an i18n error on “Week starts on”.

a8ci18n · 2017-03-07T19:39:22Z

client/my-sites/site-settings/date-time-format/time-format-option.jsx

+				/>
+				<FormSettingExplanation>
+					{ isCustom && timeFormat &&
+						translate( 'Preview: %s', {


Hi! I've found a possible matching string that has already been translated 67 times:
translate( 'Preview %s' ) ES Score: 18

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).

a8ci18n · 2017-03-07T19:39:23Z

client/my-sites/site-settings/date-time-format/start-of-week-option.jsx

+} ) =>
+<FormFieldset>
+	<FormLabel>
+		{ translate( 'Week starts on' ) }


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).

dmsnell · 2017-03-07T20:27:27Z

client/my-sites/site-settings/date-time-format/date-time-format-options.jsx

+		start_of_week: 0,
+		time_format: '',
+		timezone_string: '',
+	};


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

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

Copons · 2017-03-07T20:36:20Z

@dmsnell Moved defaultSettings away from wrapSettingsForm and into the module constants.

a8ci18n · 2017-03-07T20:37:37Z

client/my-sites/site-settings/date-time-format/time-format-option.jsx

+				/>
+				<FormSettingExplanation>
+					{ isCustom && timeFormat &&
+						translate( 'Preview: %s', {


Hi! I've found a possible matching string that has already been translated 67 times:
translate( 'Preview %s' ) ES Score: 18

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).

a8ci18n · 2017-03-07T20:37:38Z

client/my-sites/site-settings/date-time-format/start-of-week-option.jsx

+} ) =>
+<FormFieldset>
+	<FormLabel>
+		{ translate( 'Week starts on' ) }


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).

Copons added [Feature] Site Settings All other general site settings. [Status] In Progress labels Feb 8, 2017

Copons added this to the Feature Parity Audit: wp-admin milestone Feb 8, 2017

a8ci18n reviewed Feb 8, 2017

View reviewed changes

Copons mentioned this pull request Feb 10, 2017

Site Settings: add date format, time format, week starts on #10400

Merged

5 tasks

Copons force-pushed the add/site-settings/date-format-sub-page branch 2 times, most recently from 6d3b1e6 to 817c2b8 Compare February 14, 2017 14:08

Copons added a commit that referenced this pull request Feb 16, 2017

Update language option copy

4aa8e05

See: #11259 (comment)

a8ci18n reviewed Feb 16, 2017

View reviewed changes

Copons mentioned this pull request Feb 16, 2017

Site Settings: Update language option copy #11432

Merged

roundhill reviewed Feb 16, 2017

View reviewed changes

a8ci18n reviewed Feb 17, 2017

View reviewed changes

davewhitley requested changes Feb 17, 2017

View reviewed changes

davewhitley removed the [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR label Feb 17, 2017

Copons force-pushed the add/site-settings/date-format-sub-page branch from ab9e54f to a9c601b Compare February 20, 2017 13:59

davewhitley approved these changes Feb 20, 2017

View reviewed changes

a8ci18n reviewed Mar 7, 2017

View reviewed changes

Rename now as localizedDate to improve clarity

a15c407

a8ci18n reviewed Mar 7, 2017

View reviewed changes

dmsnell reviewed Mar 7, 2017

View reviewed changes

Improve code and readability

a3cf018

- Remove numeric `key={ index }`. - Remove unnecessary ternaries. - Use `lodash.pick()` to pick site settings. - Fix an i18n error on “Week starts on”.

matticbot added the [Size] XL Probably needs to be broken down into multiple smaller issues label Mar 7, 2017

a8ci18n reviewed Mar 7, 2017

View reviewed changes

dmsnell reviewed Mar 7, 2017

View reviewed changes

Make defaultSettings a module constant

7c1edeb

a8ci18n reviewed Mar 7, 2017

View reviewed changes

Copons mentioned this pull request Mar 8, 2017

Settings: design for Date Format and Time Format site settings #10441

Closed

dmsnell approved these changes Mar 13, 2017

View reviewed changes

Copons merged commit 657c487 into master Mar 14, 2017

Copons deleted the add/site-settings/date-format-sub-page branch March 14, 2017 11:54

Copons removed [Status] Needs Copy Review Add this when you'd like to get a review / feedback from the Editorial team on your PR [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Mar 14, 2017

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

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

Conversation

Copons commented Feb 8, 2017 • edited Loading

Screenshots

GIF

matticbot commented Feb 8, 2017

a8ci18n Feb 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Copons Feb 8, 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

kristastevens commented Feb 14, 2017

Copons commented Feb 14, 2017

a8ci18n Feb 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Copons commented Feb 16, 2017

yoavf commented Feb 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

a8ci18n Feb 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davewhitley commented Feb 17, 2017

Choose a reason for hiding this comment

Copons commented Mar 7, 2017

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

dmsnell Mar 7, 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

dmsnell commented Mar 7, 2017

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

Copons commented Mar 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Copons commented Feb 8, 2017 •

edited

Loading

a8ci18n Feb 8, 2017 •

edited

Loading

Copons Feb 8, 2017 •

edited

Loading

a8ci18n Feb 16, 2017 •

edited

Loading

yoavf commented Feb 16, 2017 •

edited

Loading

a8ci18n Feb 17, 2017 •

edited

Loading

dmsnell Mar 7, 2017 •

edited

Loading