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.

Sign up for GitHub

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

Jump to bottom

Site settings: updated Jetpack toggle spacing and child-settings indentation #11240

Merged
merged 5 commits into from
Feb 10, 2017
Merged

Site settings: updated Jetpack toggle spacing and child-settings indentation #11240

merged 5 commits into from
Feb 10, 2017

Conversation

MichaelArestad
Copy link
Contributor

@MichaelArestad MichaelArestad commented Feb 7, 2017 •
edited
Loading

As per @rickybanister's good catch in #11237

Changes

  • changed module toggle spacing from 8px to 12px to be consistent with other toggles
  • changed child settings indent spacing from 32px to 36px
  • factored out repeated child settings styles to just use a single style

Before

image

After

image

@MichaelArestad MichaelArestad self-assigned this Feb 7, 2017
@matticbot
Copy link
Contributor

Test live: https://calypso.live/?branch=fix/toggle-indent-spacing

@MichaelArestad MichaelArestad added [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 7, 2017
@MichaelArestad MichaelArestad requested a review from ryelle February 7, 2017 19:13
@MichaelArestad
Copy link
Contributor Author

CC @tyxla and @oskosk

ryelle
ryelle reviewed Feb 7, 2017
View reviewed changes
Copy link
Member

@ryelle ryelle left a comment

Choose a reason for hiding this comment

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

Looks good, I just have a few nitpicky comments :)

client/my-sites/site-settings/custom-content-types/index.jsx Outdated
@@ -99,7 +99,7 @@ class CustomContentTypes extends Component {
const { customContentTypesModuleActive } = this.props;

return (
<div className="custom-content-types__module-settings is-indented">
<div className="site-settings__child-settings">
Copy link
Member

Choose a reason for hiding this comment

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

Using only this classname is flagging the JSX class namespace rule, because there is no prefixed classname here. If you change this to custom-content-types__module-settings site-settings__child-settings, it'll pass. (same with the other places this was replaced)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this being flagged for all the settings groups or just this one?

Copy link
Member

Choose a reason for hiding this comment

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

All of them - it should be fine if you bring back the *-settings class for each group.

client/my-sites/site-settings/jetpack-module-toggle/style.scss Outdated
@@ -1,7 +1,7 @@
.jetpack-module-toggle .form-toggle__label *:not( .form-toggle__switch ) {
margin-left: 8px;
margin-left: 12px;
Copy link
Member

Choose a reason for hiding this comment

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

Can this just be removed, if we're resetting it to the same as the general toggle spacing?

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 think so. I couldn't find anything it effected.

Copy link
Member

Choose a reason for hiding this comment

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

I confirm, this one can safely be removed now.

tyxla
tyxla approved these changes Feb 8, 2017
View reviewed changes
Copy link
Member

@tyxla tyxla left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks great in my testing, and @ryelle said it all in her review. Once her feedback is addressed, I think this one should be ready to go. Thanks for fixing those 👍

@tyxla tyxla mentioned this pull request Feb 8, 2017
Merged
@tyxla tyxla mentioned this pull request Feb 9, 2017
Merged
@ryelle ryelle force-pushed the fix/toggle-indent-spacing branch from 63ef41e to 9a6eddf Compare February 9, 2017 16:24
@ryelle
Copy link
Member

ryelle commented Feb 9, 2017

I've rebased the PR & fixed the code style notes, @oskosk @tyxla can you give this another 👀 ?

@tyxla
Copy link
Member

tyxla commented Feb 10, 2017

@ryelle this looks awesome! Let's ship it 🚢 !

@tyxla tyxla added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Feb 10, 2017
@ryelle ryelle merged commit 0e0623e into master Feb 10, 2017
@ryelle ryelle deleted the fix/toggle-indent-spacing branch February 10, 2017 16:13
@ryelle ryelle mentioned this pull request Feb 10, 2017
Merged
tyxla added a commit that referenced this pull request Feb 11, 2017
@tyxla
Site Settings: Update Composing styles as implemented in #11240
76aa007
tyxla added a commit that referenced this pull request Feb 14, 2017
@tyxla
Site Settings: Update Composing styles as implemented in #11240
cd4e723
tyxla added a commit that referenced this pull request Feb 14, 2017
@tyxla
Site Settings: Add more settings Composing card for Jetpack sites (#1…
0f7e76b 
…1231)

* Site Settings: Move wpcom markdown in a separate component

* Site Settings: Move default post format in a separate component

* Site Settings: Initial version of After the Deadline settings

* Site Settings: Improve translations in AfterTheDeadline

* Site Settings: Enable AfterTheDeadline only for supported sites

* Site Settings: Add support for manually setting field value in wrapSettingsForm

* Site Settings: Use TokenField instead of FormTextInput for ignored phrases

* Site Settings: Rename MarkdownWpcom to just Markdown

* Site Settings: Rename MarkdownWpcom to just Markdown

* Site Settings: Improve wording in AfterTheDeadline labels

* Site Settings: Cleanup unused props in Composing card

* Site Settings: Implemenet advanced settings toggle in AfterTheDeadline

* State: Add AfterTheDeadline settings to filterSettingsByActiveModules

* Site Settings: Update Composing styles as implemented in #11240

* Site Settings: Formatting and alignment improvements in After The Deadline card
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ryelle ryelle ryelle left review comments

@tyxla tyxla tyxla approved these changes

Assignees

@MichaelArestad MichaelArestad

Labels
[Feature] Site Settings All other general site settings.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@MichaelArestad @matticbot @ryelle @tyxla @oskosk