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 more settings Composing card for Jetpack sites #11231

Merged
merged 15 commits into from
Feb 14, 2017

Conversation

tyxla
Copy link
Member

@tyxla tyxla commented Feb 7, 2017

This PR implements After the Deadline settings for Jetpack sites in the Writing > Composing card. It also moves the current Markdown and Default Post Format settings in separate components in order to keep the main Composing card component clean. It also introduces a setFieldValue method to wrapSettingsForm that allows manually setting a specific field's value. The PR is part of #9171.

Preview (with collapsed Advanced settings):

Preview (with expanded Advanced settings):

To test:

  • Checkout this branch
  • Go to /settings/writing/$site where $site is one of your Jetpack sites.
  • Verify the default post format field works properly like before.
  • Verify the Markdown field is not displayed.
  • Play with the new After the Deadline settings (main toggle and sub settings) and verify they are stored and retrieved properly.
  • Go to /settings/writing/$site where $site is one of your WordPress.com sites.
  • Verify the default post format field works properly like before.
  • Verify the Markdown field works properly like before.
  • Verify the After the Deadline settings are not displayed for this site.

Styles here could benefit from #11240 - I can update the PR once it's merged.

@tyxla tyxla added this to the Jetpack Module Settings in Calypso milestone Feb 7, 2017
@tyxla tyxla self-assigned this Feb 7, 2017
@matticbot
Copy link
Contributor

matticbot commented Feb 7, 2017

@oskosk oskosk mentioned this pull request Feb 7, 2017
55 tasks
<JetpackModuleToggle
siteId={ selectedSiteId }
moduleSlug="after-the-deadline"
label={ translate( 'Check your spelling, style, and grammar.' ) }
Copy link

@a8ci18n a8ci18n Feb 7, 2017

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 23 times:
translate( 'Spelling and Grammar' ) ES Score: 8.15

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

{ translate( 'English Options' ) }
</FormLegend>
<FormSettingExplanation>
{ translate( 'Enable proofreading for the following grammar and style rules:' ) }
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 40 times:
translate( 'Enable proofreading for the following grammar and style rules when writing posts and pages:' ) ES Score: 15.58

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

{ translate( 'Automatic Language Detection' ) }
</FormLegend>
<FormSettingExplanation>
{ translate( 'The proofreader supports English, French, German, Portuguese and Spanish.' ) }
Copy link

@a8ci18n a8ci18n Feb 7, 2017

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 19 times:
translate( 'The proofreader supports English, French, German, Portuguese, and Spanish. Your <a href="options-general.php">blog language</a> setting is the default proofreading language.' ) ES Score: 11.37

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


{
this.renderToggle( 'onupdate', ! afterTheDeadlineModuleActive, translate(
'A post or page is updated'
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 47 times:
translate( 'a post or page is updated' ) ES Score: 12.39

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


{
this.renderToggle( 'onpublish', ! afterTheDeadlineModuleActive, translate(
'A post or page is first published'
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 45 times:
translate( 'a post or page is first published' ) ES Score: 13.52

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

@tyxla tyxla added [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Feb 8, 2017
@rickybanister
Copy link

This looks good. I imagine the other PR to standardize the right-margin of toggles will take care of the misalignment here.

The only other thing I noticed was the lack of initial capitals on the first two proofreading settings:

screen shot 2017-02-08 at 10 21 38 am

All our other labels have an initial capital letter, we should standardize.

@rickybanister
Copy link

To follow up, if we don't like that those two labels start with a... we could change them to be plural:

Posts or pages are first published
Posts or pages are updated

@tyxla tyxla force-pushed the add/jetpack-settings-composing branch from a610118 to 50e1091 Compare February 9, 2017 11:51
@tyxla
Copy link
Member Author

tyxla commented Feb 9, 2017

This looks good. I imagine the other PR to standardize the right-margin of toggles will take care of the misalignment here.

Yes - I'll make sure to do any necessary updates/cleanups here once that PR lands.

To follow up, if we don't like that those two labels start with a... we could change them to be plural:

Even better! I've updated the labels to the one you suggested.

@rickybanister do you feel there's anything else we should improve, design-wise? Also cc @MichaelArestad for a second opinion.


{
this.renderToggle( 'onupdate', ! afterTheDeadlineModuleActive, translate(
'Posts or pages are updated'
Copy link

@a8ci18n a8ci18n Feb 9, 2017

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 47 times:
translate( 'a post or page is updated' ) ES Score: 7.10

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


{
this.renderToggle( 'onpublish', ! afterTheDeadlineModuleActive, translate(
'Posts or pages are first published'
Copy link

@a8ci18n a8ci18n Feb 9, 2017

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 45 times:
translate( 'a post or page is first published' ) ES Score: 9.01

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

@MichaelArestad
Copy link
Contributor

@tyxla We should hide the majority of those settings in an "Advanced options" section like we are doing in the feature/settings-overhaul branch of Jetpack:

image

@tyxla
Copy link
Member Author

tyxla commented Feb 9, 2017

@MichaelArestad of course! Thanks for pointing that out. It's now implemented in 202a41a, I've also updated the previews in the PR description. Feel free to have another 👁 .

@@ -0,0 +1,7 @@
.composing__module-settings.is-indented {
margin: 16px 32px 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we drop this in favor of the solution offered here: #11240

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, thanks for pointing that out. I've already added that to the description of the PR, and I'm planning to address it after it's merged.

Copy link
Member Author

Choose a reason for hiding this comment

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

@MichaelArestad this has been implemented in 76aa007.

@tyxla tyxla force-pushed the add/jetpack-settings-composing branch from 76aa007 to c757244 Compare February 14, 2017 09:26
@tyxla
Copy link
Member Author

tyxla commented Feb 14, 2017

@rickybanister minor things addressed. A small preview of the updated version:

Feel free to have another look.

@tyxla tyxla added [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR and removed [Status] Ready to Merge labels Feb 14, 2017
<JetpackModuleToggle
siteId={ selectedSiteId }
moduleSlug="after-the-deadline"
label={ translate( 'Check your spelling, style, and grammar' ) }
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 23 times:
translate( 'Spelling and Grammar' ) ES Score: 8.15

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

@rickybanister
Copy link

@tyxla beautiful, ship it.

@rickybanister rickybanister added [Status] Ready to Merge and removed [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR labels Feb 14, 2017
@folletto
Copy link
Contributor

Sorry if I missed this before, but wouldn't be better here to use our foldable card pattern instead of a link to open "Advanced settings"?

Something like this:

foldable-spelling-card

@MichaelArestad
Copy link
Contributor

@folletto That's been brought up recently by @rickybanister as well. It's something we're definitely exploring, but this works and is good to merge for now.

@tyxla tyxla force-pushed the add/jetpack-settings-composing branch from 40e77be to cf4f59c Compare February 14, 2017 11:09
@tyxla tyxla merged commit 0f7e76b into master Feb 14, 2017
@tyxla tyxla deleted the add/jetpack-settings-composing branch February 14, 2017 11:20
@oskosk oskosk mentioned this pull request Mar 1, 2017
48 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Site Settings All other general site settings. Jetpack
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants