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: Update all sections to use SectionHeader #1898

Merged
merged 5 commits into from Mar 3, 2016

Conversation

Projects
None yet
8 participants
@alternatekev
Contributor

alternatekev commented Dec 21, 2015

This PR updates Writing, Discussion, & Analytics settings pages to use the SectionHeader.
General Settings (just puts Jetpack at the bottom for Jetpack blogs)
screen shot 2016-02-01 at 2 42 20 pm

Writing Settings:
writing

Discussion Settings
discussion

Analytics Settings
analytics

Security Settings
security

Show outdated Hide outdated client/my-sites/site-settings/section-writing.jsx
<WritingForm site={ this.props.site } />
</Card>
<WritingForm site={ this.props.site } />

This comment has been minimized.

@dmsnell

dmsnell Jan 5, 2016

Contributor

In cases where we have a single return value that isn't a long line, I personally prefer to eliminate the parentheses.

render: function() {
    return <WritingForm site={ this.props.site } />;
}
@dmsnell

dmsnell Jan 5, 2016

Contributor

In cases where we have a single return value that isn't a long line, I personally prefer to eliminate the parentheses.

render: function() {
    return <WritingForm site={ this.props.site } />;
}

This comment has been minimized.

@dmsnell

dmsnell Jan 5, 2016

Contributor

Also, am I missing something big here? Does this component have more-to-come because it looks like it doesn't do anything but print out a debug statement.

If it doesn't have more coming, can we not replace it altogether with the <WritingForm />

@dmsnell

dmsnell Jan 5, 2016

Contributor

Also, am I missing something big here? Does this component have more-to-come because it looks like it doesn't do anything but print out a debug statement.

If it doesn't have more coming, can we not replace it altogether with the <WritingForm />

This comment has been minimized.

@dmsnell

dmsnell Jan 5, 2016

Contributor

Card is no longer necessary after these edits, so it should be removed in Line 10

@dmsnell

dmsnell Jan 5, 2016

Contributor

Card is no longer necessary after these edits, so it should be removed in Line 10

Show outdated Hide outdated client/my-sites/site-settings/section-writing.jsx
</Card>
<WritingForm site={ this.props.site } />
);
}

This comment has been minimized.

@dmsnell

dmsnell Jan 5, 2016

Contributor

As simple as this component is, would you mind updating it to use ES6 syntax (if the component is here to stay)? Nothing too much would change except for using import instead of var X = require( Y ); and module.exports could be changed to export default React.createClass( … ). We could also use the function short-hand like render() { … } instead of render: function() { … }

@dmsnell

dmsnell Jan 5, 2016

Contributor

As simple as this component is, would you mind updating it to use ES6 syntax (if the component is here to stay)? Nothing too much would change except for using import instead of var X = require( Y ); and module.exports could be changed to export default React.createClass( … ). We could also use the function short-hand like render() { … } instead of render: function() { … }

@dmsnell

This comment has been minimized.

Show comment
Hide comment
@dmsnell

dmsnell Jan 5, 2016

Contributor

@alternatekev the code looks mostly good but I had questions about the section-writing component.

There are also several other aesthetic issues that could be improved here but they belong in another PR. These are things like code formatting, splitting translation strings into multiple lines, etc…

If nobody has volunteered to do a design review yet, then it would be appropriate to ping someone directly and ask for their help. Please feel free to ping me again if this sits in queue again for an extended period.

Contributor

dmsnell commented Jan 5, 2016

@alternatekev the code looks mostly good but I had questions about the section-writing component.

There are also several other aesthetic issues that could be improved here but they belong in another PR. These are things like code formatting, splitting translation strings into multiple lines, etc…

If nobody has volunteered to do a design review yet, then it would be appropriate to ping someone directly and ask for their help. Please feel free to ping me again if this sits in queue again for an extended period.

@alternatekev

This comment has been minimized.

Show comment
Hide comment
@alternatekev

alternatekev Jan 15, 2016

Contributor

@dmsnell this should be good for another review now

Contributor

alternatekev commented Jan 15, 2016

@dmsnell this should be good for another review now

@adambbecker

This comment has been minimized.

Show comment
Hide comment
@adambbecker

adambbecker Jan 15, 2016

Member

@alternatekev: Design wise, this is looking nice (focusing on the section header stuff)! Good update for sure consistency wise. It's definitely better to have all the sections looking similar. Even though, as @dmsnell mentioned there are lots of other improvements to be made overall, focusing on just the headers for this PR makes sense.

Jetpack option added

One thing I did notice that was a bit out of the section header scope was the addition of this Jetpack site option.

screen shot 2016-01-15 at 4 21 24 pm

Personally, I think we could leave that addition out of this current PR mostly because it could be split out to include it's own PR which could include some tweaks such as:

Messaging copy pass

Seems a little unclear to me on first read, could be something like (if I'm understanding the option correctly):

Sync all posts & pages
By default, only "published" posts & pages are synced.

Option testing

I did check / uncheck (& save) this option and it seemed to sync all the posts / pages (drafts, scheduled, etc.) for the Jetpack site regardless of whether this option was checked. So I'm not entirely sure what the unchecked state actually does.

Section header multiple buttons

I noticed there was a spacing solution added to the section header button (disconnect in this case) that could probably be expanded for all section headers. Margin by default to account for multiple buttons or button groups with the last child removing said margin, etc.


@rralian: Don't know if you wanted to take a look at the code here real quick being the OG for settings code.

Member

adambbecker commented Jan 15, 2016

@alternatekev: Design wise, this is looking nice (focusing on the section header stuff)! Good update for sure consistency wise. It's definitely better to have all the sections looking similar. Even though, as @dmsnell mentioned there are lots of other improvements to be made overall, focusing on just the headers for this PR makes sense.

Jetpack option added

One thing I did notice that was a bit out of the section header scope was the addition of this Jetpack site option.

screen shot 2016-01-15 at 4 21 24 pm

Personally, I think we could leave that addition out of this current PR mostly because it could be split out to include it's own PR which could include some tweaks such as:

Messaging copy pass

Seems a little unclear to me on first read, could be something like (if I'm understanding the option correctly):

Sync all posts & pages
By default, only "published" posts & pages are synced.

Option testing

I did check / uncheck (& save) this option and it seemed to sync all the posts / pages (drafts, scheduled, etc.) for the Jetpack site regardless of whether this option was checked. So I'm not entirely sure what the unchecked state actually does.

Section header multiple buttons

I noticed there was a spacing solution added to the section header button (disconnect in this case) that could probably be expanded for all section headers. Margin by default to account for multiple buttons or button groups with the last child removing said margin, etc.


@rralian: Don't know if you wanted to take a look at the code here real quick being the OG for settings code.

@dmsnell

This comment has been minimized.

Show comment
Hide comment
@dmsnell

dmsnell Jan 15, 2016

Contributor

@alternatekev thanks for the updates. I like this and think that from a code-perspective we are good-to-go

Contributor

dmsnell commented Jan 15, 2016

@alternatekev thanks for the updates. I like this and think that from a code-perspective we are good-to-go

@alternatekev

This comment has been minimized.

Show comment
Hide comment
@alternatekev

alternatekev Jan 15, 2016

Contributor

Can someone verify for me whether the Jetpack Scan "Save Settings" button works when you have correct SFTP information there? When I do it, I get an indeterminate spinner that never goes away:

screen shot 2016-01-15 at 3 52 27 pm

Contributor

alternatekev commented Jan 15, 2016

Can someone verify for me whether the Jetpack Scan "Save Settings" button works when you have correct SFTP information there? When I do it, I get an indeterminate spinner that never goes away:

screen shot 2016-01-15 at 3 52 27 pm

@enejb

This comment has been minimized.

Show comment
Hide comment
@enejb

enejb Jan 15, 2016

Member

@roccotripaldi ^^. @alternatekev I think the whole process of verifying the Jetpack Scan credentials a bit more complicated. I think @roccotripaldi should know more about it.

Member

enejb commented Jan 15, 2016

@roccotripaldi ^^. @alternatekev I think the whole process of verifying the Jetpack Scan credentials a bit more complicated. I think @roccotripaldi should know more about it.

@alternatekev

This comment has been minimized.

Show comment
Hide comment
@alternatekev

alternatekev Jan 15, 2016

Contributor

@adambbecker Yep that code isn't available on production. This PR doesn't enable it. I had some problems because of that with earlier updates of this page.

Essentially: that copy/UI won't really show up at all until the feature is ready to go. The only button you get until then is Disconnect.

I noticed there was a spacing solution added to the section header button (disconnect in this case) that could probably be expanded for all section headers.

Totally agree on that one. I can do a new PR for SectionHeader to fix that,

Contributor

alternatekev commented Jan 15, 2016

@adambbecker Yep that code isn't available on production. This PR doesn't enable it. I had some problems because of that with earlier updates of this page.

Essentially: that copy/UI won't really show up at all until the feature is ready to go. The only button you get until then is Disconnect.

I noticed there was a spacing solution added to the section header button (disconnect in this case) that could probably be expanded for all section headers.

Totally agree on that one. I can do a new PR for SectionHeader to fix that,

@alternatekev

This comment has been minimized.

Show comment
Hide comment
@alternatekev

alternatekev Jan 15, 2016

Contributor

So it looks like:

  • There's an uncaught exception with Jetpack Scan and servers that have a problem with it (like mine)
  • When the connection does work, it brings back a pretty wonky UI as provided in a screenshot taken by @ebinnion:

screen_shot_720

@enejb @roccotripaldi It doesn't look like this feature is enabled on Production. Is that correct? Can we disable these UIs in the non-production environments, too, until they're ready? And by "they" I mean:

  • Jetpack Scan
  • Non-public Post syncing
Contributor

alternatekev commented Jan 15, 2016

So it looks like:

  • There's an uncaught exception with Jetpack Scan and servers that have a problem with it (like mine)
  • When the connection does work, it brings back a pretty wonky UI as provided in a screenshot taken by @ebinnion:

screen_shot_720

@enejb @roccotripaldi It doesn't look like this feature is enabled on Production. Is that correct? Can we disable these UIs in the non-production environments, too, until they're ready? And by "they" I mean:

  • Jetpack Scan
  • Non-public Post syncing
@ebinnion

This comment has been minimized.

Show comment
Hide comment
@ebinnion

ebinnion Jan 15, 2016

Contributor

@alternatekev - For what it's worth, I don't think we need to disable the UIs in non-production environments. At least for Jetpack scan, we could probably just not update it to use section-header for now.

Contributor

ebinnion commented Jan 15, 2016

@alternatekev - For what it's worth, I don't think we need to disable the UIs in non-production environments. At least for Jetpack scan, we could probably just not update it to use section-header for now.

@rickybanister

This comment has been minimized.

Show comment
Hide comment
@rickybanister

rickybanister Jan 27, 2016

Member

Jetpack security icons look nice. Hope that stuff can get some attention soon.

Member

rickybanister commented Jan 27, 2016

Jetpack security icons look nice. Hope that stuff can get some attention soon.

@adambbecker

This comment has been minimized.

Show comment
Hide comment
@adambbecker

adambbecker Jan 28, 2016

Member

@alternatekev: Looking good and I definitely agree w/ @rickybanister about the Jetpack security icons, those go a long way.

Seeing everything in action does make me really wish the form sections were more separated, but I understand why they are not currently: save buttons not actually being separate saves, etc.. But that being said, do you think it's worth trying the technique where there are separated sections, but not all the "save" buttons are immediately visible? They would only become visible if a setting in that section was changed. That would hopefully cut down on the primary button clutter a bit while still having the smaller, separated, and focused forms.

Member

adambbecker commented Jan 28, 2016

@alternatekev: Looking good and I definitely agree w/ @rickybanister about the Jetpack security icons, those go a long way.

Seeing everything in action does make me really wish the form sections were more separated, but I understand why they are not currently: save buttons not actually being separate saves, etc.. But that being said, do you think it's worth trying the technique where there are separated sections, but not all the "save" buttons are immediately visible? They would only become visible if a setting in that section was changed. That would hopefully cut down on the primary button clutter a bit while still having the smaller, separated, and focused forms.

@alternatekev

This comment has been minimized.

Show comment
Hide comment
@alternatekev

alternatekev Feb 1, 2016

Contributor

do you think it's worth trying the technique where there are separated sections, but not all the "save" buttons are immediately visible? They would only become visible if a setting in that section was changed.

@mtias & @adambbecker I've updated this PR to walk back the General Settings so they're back in separate Cards. All this PR does to that page now is move Jetpack Settings to the very bottom. Take a look and we can get a new PR started for the hide/show buttons interaction.

Contributor

alternatekev commented Feb 1, 2016

do you think it's worth trying the technique where there are separated sections, but not all the "save" buttons are immediately visible? They would only become visible if a setting in that section was changed.

@mtias & @adambbecker I've updated this PR to walk back the General Settings so they're back in separate Cards. All this PR does to that page now is move Jetpack Settings to the very bottom. Take a look and we can get a new PR started for the hide/show buttons interaction.

@mtias

This comment has been minimized.

Show comment
Hide comment
@mtias

mtias Feb 12, 2016

Member

@alternatekev sorry for the delay, will be looking at this today.

Member

mtias commented Feb 12, 2016

@alternatekev sorry for the delay, will be looking at this today.

alternatekev added some commits Dec 21, 2015

changed writing settings to sectionheader
changed discussion settings to sectionheader

changed analytics settings to sectionheader

updated jetpack security screen

fixed jp protect activate buttons

moved loader into sectionheader

changed writing settings to sectionheader

changed discussion settings to sectionheader

changed analytics settings to sectionheader

fixed jp protect activate buttons

moved loader into sectionheader

component cleanup via @dmsnell

removed extaneous section-writing.jsx file

changed writing settings to sectionheader

updated jetpack security screen

fixed jp protect activate buttons

moved loader into sectionheader

changed writing settings to sectionheader

changed discussion settings to sectionheader

changed analytics settings to sectionheader

fixed jp protect activate buttons

moved loader into sectionheader

component cleanup via @dmsnell

removed extaneous section-writing.jsx file

added divider lines and remixed the general settings a bit
adding branding imagery to security settings
changed writing settings to sectionheader

changed discussion settings to sectionheader

changed analytics settings to sectionheader

component cleanup via @dmsnell

removed extaneous section-writing.jsx file

changed writing settings to sectionheader

updated jetpack security screen

fixed jp protect activate buttons

moved loader into sectionheader

changed writing settings to sectionheader

changed discussion settings to sectionheader

changed analytics settings to sectionheader

fixed jp protect activate buttons

moved loader into sectionheader

component cleanup via @dmsnell

removed extaneous section-writing.jsx file

added divider lines and remixed the general settings a bit

adding branding imagery to security settings

changed writing settings to sectionheader

changed discussion settings to sectionheader

changed analytics settings to sectionheader

updated jetpack security screen

fixed jp protect activate buttons

moved loader into sectionheader

changed writing settings to sectionheader

changed discussion settings to sectionheader

changed analytics settings to sectionheader

fixed jp protect activate buttons

moved loader into sectionheader

component cleanup via @dmsnell

removed extaneous section-writing.jsx file

changed writing settings to sectionheader

updated jetpack security screen

fixed jp protect activate buttons

moved loader into sectionheader

changed writing settings to sectionheader

changed discussion settings to sectionheader

changed analytics settings to sectionheader

fixed jp protect activate buttons

moved loader into sectionheader

component cleanup via @dmsnell

removed extaneous section-writing.jsx file

moved general settings back to 3 cards. moved jetpack card to the bottom

changed writing settings to sectionheader

changed discussion settings to sectionheader

changed analytics settings to sectionheader

updated jetpack security screen

fixed jp protect activate buttons

moved loader into sectionheader

changed writing settings to sectionheader

changed discussion settings to sectionheader

changed analytics settings to sectionheader

fixed jp protect activate buttons

moved loader into sectionheader

component cleanup via @dmsnell

removed extaneous section-writing.jsx file

changed writing settings to sectionheader

updated jetpack security screen

fixed jp protect activate buttons

moved loader into sectionheader

changed writing settings to sectionheader

changed discussion settings to sectionheader

changed analytics settings to sectionheader

fixed jp protect activate buttons

moved loader into sectionheader

component cleanup via @dmsnell

removed extaneous section-writing.jsx file

added divider lines and remixed the general settings a bit

adding branding imagery to security settings

moved general settings back to 3 cards. moved jetpack card to the bottom

removed extraneous related posts card as result of merge conflicts

fixed some eslint errors

fixed jetpack scan module rendering issues with sectionheader

formatting changes

formatting changes
@alternatekev

This comment has been minimized.

Show comment
Hide comment
@alternatekev

alternatekev Mar 2, 2016

Contributor

@mtias & @dmsnell this one should be ready to be reviewed at again.

Contributor

alternatekev commented Mar 2, 2016

@mtias & @dmsnell this one should be ready to be reviewed at again.

@@ -113,8 +116,8 @@ module.exports = React.createClass( {
otherCommentSettings: function() {
return (
<FormFieldset>
<FormLegend>{ this.translate( 'Other comment settings' ) }</FormLegend>
<FormFieldset className="has-divider">

This comment has been minimized.

@ebinnion

ebinnion Mar 3, 2016

Contributor

I'm not sure that we need to address it in this PR, but maybe we should consider adding a prop instead of directly applying the has-divider class.

@ebinnion

ebinnion Mar 3, 2016

Contributor

I'm not sure that we need to address it in this PR, but maybe we should consider adding a prop instead of directly applying the has-divider class.

@ebinnion

This comment has been minimized.

Show comment
Hide comment
@ebinnion

ebinnion Mar 3, 2016

Contributor

The only thing that really stood out to me was that all of the analytics tracking is causing ESLint warnings because they use bind. I attempted to fix these, but after realizing how many lines would need to be changed, I think we should do that in a separate PR.

I did go ahead and address a few other ESLint issues such as unused variables and ternary style.

I left one comment about potentially using a hasDivider prop instead of applying the has-divider class to each component. I don't think we need to address it in this PR, but something to consider.

If you're fine with my changes, go ahead and 🚢 👍

Contributor

ebinnion commented Mar 3, 2016

The only thing that really stood out to me was that all of the analytics tracking is causing ESLint warnings because they use bind. I attempted to fix these, but after realizing how many lines would need to be changed, I think we should do that in a separate PR.

I did go ahead and address a few other ESLint issues such as unused variables and ternary style.

I left one comment about potentially using a hasDivider prop instead of applying the has-divider class to each component. I don't think we need to address it in this PR, but something to consider.

If you're fine with my changes, go ahead and 🚢 👍

alternatekev added a commit that referenced this pull request Mar 3, 2016

Merge pull request #1898 from Automattic/update/site-settings-section…
…-headers

Site Settings: Update all sections to use SectionHeader

@alternatekev alternatekev merged commit 43eb119 into master Mar 3, 2016

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@ebinnion ebinnion deleted the update/site-settings-section-headers branch Mar 3, 2016

ockham added a commit that referenced this pull request Jun 1, 2016

Site Settings: Remove SettingsCardFooter
This has been obsolete as of #1898.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment