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

Updates module settings positions and renames tabs. #5405

Merged
merged 7 commits into from Nov 3, 2016

Conversation

zinigor
Copy link
Member

@zinigor zinigor commented Oct 24, 2016

Changes proposed in this PR

  • Introduced two new tabs: Discussion and Traffic
  • Removed General, Appearance and Engagement tabs
  • Rearranged module settings according to the new tabs

Testing

  • Make sure all module cards work as before as an admin.
  • Make sure all module cards and tabs work as before as a non-admin.
  • Make sure things work in development mode.
  • Make sure things work with active Jetpack plans.

Related discussion: p6TEKc-HQ-p2

@zinigor zinigor added [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Status] In Progress Admin Page React-powered dashboard under the Jetpack menu labels Oct 24, 2016
@zinigor zinigor added this to the 4.4 milestone Oct 24, 2016
@zinigor zinigor self-assigned this Oct 24, 2016
@zinigor zinigor added [Status] Needs Review To request a review from Crew. Label will be renamed soon. and removed [Status] In Progress labels Oct 26, 2016
@MichaelArestad MichaelArestad added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Oct 26, 2016
Copy link
Contributor

@eliorivero eliorivero left a comment

Choose a reason for hiding this comment

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

There are things like Infinite Scroll that should be in Traffic.
Maybe Mobile Theme should be in that tab too.

@@ -169,33 +168,28 @@ const Main = React.createClass( {
break;
case '/settings':
Copy link
Contributor

Choose a reason for hiding this comment

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

This /settings case and /writing can be 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.

Thanks, it totally slipped my mind that it could have been done this way.

path="#general"
selected={ ( this.props.route.path === '/general' || this.props.route.path === '/settings' ) }>
{ __( 'General', { context: 'Navigation item.' } ) }
path="#settings"
Copy link
Contributor

Choose a reason for hiding this comment

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

It's counter intuitive that this path is #settings. It should be #writing.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, fixed that, plus fixed the view for authors.

selected={ ( this.props.route.path === '/general' || this.props.route.path === '/settings' ) }>
{ __( 'General', { context: 'Navigation item.' } ) }
path="#settings"
selected={ ( this.props.route.path === '/settings' || this.props.route.path === '/settings' ) }>
Copy link
Contributor

Choose a reason for hiding this comment

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

This condition should check if it's /writing or /settings.

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!

selected={ ( this.props.route.path === '/general' || this.props.route.path === '/settings' ) }>
{ __( 'General', { context: 'Navigation item.' } ) }
path="#settings"
selected={ ( this.props.route.path === '/settings' || this.props.route.path === '/settings' ) }>
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comments than above about path and condition

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!

selected={ this.props.route.path === '/engagement' }>
{ __( 'Engagement', { context: 'Navigation item.' } ) }
path="#settings"
selected={ ( this.props.route.path === '/settings' || this.props.route.path === '/settings' ) }>
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comments than above about path and condition

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!

@eliorivero eliorivero added [Status] Needs Review To request a review from Crew. Label will be renamed soon. [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Oct 26, 2016
@samhotchkiss
Copy link
Contributor

#5461

@eliorivero eliorivero removed the [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! label Nov 2, 2016
@zinigor zinigor added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Nov 3, 2016
@zinigor zinigor merged commit 5b9a9f5 into master Nov 3, 2016
@zinigor zinigor deleted the update/module-placement branch November 3, 2016 20:00
jeherve added a commit that referenced this pull request Nov 10, 2016
Changelog: add #5186

Changelog: add #3425

Changelog: add #5365

Changelog: add #5428

Changelog: add #3358

Changelog: add #5183

Changelog: add #4881

Changelog: add #5286

Changelog: add #5395

Changelog: add #5419

Changelog: add #5437

Changelog: add #5291

Update version number

Changelog: add #5476, #5413, #5409, #5355, #5348

Changelog: add #5381

Changelog: add #5412

Changelog: add #5386

Changelog: add #5250

CHangelog: add #5011

Changelog: add #5090

Changelog: add IDC fixes.

Changelog: add #5259

CHangelog: add #5186

Changelog: add #5236

Changelog: add #5284

Changelog: add #5366

Changelog: add #5382

Changelog: add #5396

Changelog: add #5405

Changelog: add #4897

Changelog: add #5289

Added a changelog entry about #5534.

Added a changelog entry about #5479.

Added a changelog entry about #5454.

Added a changelog entry about #5434.

Added a changelog entry about #5408.

Added a changelog entry about #5369.

Added a changelog entry about #5350.

Added a changelog entry about #5324.

Added a changelog entry about #5319.

Added a changelog entry about #5310.

Added a changelog entry about #5282.

Added a changelog entry about #5176.

Added a changelog entry about #3515.

Added a changelog entry about #1542.

Added a changelog entry about #5316.

Added a changelog entry about #3188.

Changelog: add #4987

Changelog: add #5270

Changelog: add #5225

Changelog: add #5507

Changelog: add #5432

Changelog: add #5473

Changelog: add #5392

Changelog: add #5222

Changelog: add #5457

Changelog: add #5423

Changelog: add #5332

Changelog: add #3853

Changelog: add #5237

Changelog: add #5307

Changelog: move up release headliner.

Changelog: add #5375

CHangelog: add #5496

Changelog: add #5528

Changelog: add #5537

Changelog: remove new Settings, they're punted to 4.5.

Changelog: move release headliner to the top.

Changelog: add testing list.

Changelog: add #4953

Changelog: add #5575

Changelog: add #5573

Changelog: add #5345
zinigor pushed a commit that referenced this pull request Nov 10, 2016
* Changelog: add PRs belonging to 4.4.

Changelog: add #5186

Changelog: add #3425

Changelog: add #5365

Changelog: add #5428

Changelog: add #3358

Changelog: add #5183

Changelog: add #4881

Changelog: add #5286

Changelog: add #5395

Changelog: add #5419

Changelog: add #5437

Changelog: add #5291

Update version number

Changelog: add #5476, #5413, #5409, #5355, #5348

Changelog: add #5381

Changelog: add #5412

Changelog: add #5386

Changelog: add #5250

CHangelog: add #5011

Changelog: add #5090

Changelog: add IDC fixes.

Changelog: add #5259

CHangelog: add #5186

Changelog: add #5236

Changelog: add #5284

Changelog: add #5366

Changelog: add #5382

Changelog: add #5396

Changelog: add #5405

Changelog: add #4897

Changelog: add #5289

Added a changelog entry about #5534.

Added a changelog entry about #5479.

Added a changelog entry about #5454.

Added a changelog entry about #5434.

Added a changelog entry about #5408.

Added a changelog entry about #5369.

Added a changelog entry about #5350.

Added a changelog entry about #5324.

Added a changelog entry about #5319.

Added a changelog entry about #5310.

Added a changelog entry about #5282.

Added a changelog entry about #5176.

Added a changelog entry about #3515.

Added a changelog entry about #1542.

Added a changelog entry about #5316.

Added a changelog entry about #3188.

Changelog: add #4987

Changelog: add #5270

Changelog: add #5225

Changelog: add #5507

Changelog: add #5432

Changelog: add #5473

Changelog: add #5392

Changelog: add #5222

Changelog: add #5457

Changelog: add #5423

Changelog: add #5332

Changelog: add #3853

Changelog: add #5237

Changelog: add #5307

Changelog: move up release headliner.

Changelog: add #5375

CHangelog: add #5496

Changelog: add #5528

Changelog: add #5537

Changelog: remove new Settings, they're punted to 4.5.

Changelog: move release headliner to the top.

Changelog: add testing list.

Changelog: add #4953

Changelog: add #5575

Changelog: add #5573

Changelog: add #5345

* Changelog: add #5168
samhotchkiss pushed a commit that referenced this pull request Nov 11, 2016
* Changelog: add PRs belonging to 4.4.

Changelog: add #5186

Changelog: add #3425

Changelog: add #5365

Changelog: add #5428

Changelog: add #3358

Changelog: add #5183

Changelog: add #4881

Changelog: add #5286

Changelog: add #5395

Changelog: add #5419

Changelog: add #5437

Changelog: add #5291

Update version number

Changelog: add #5476, #5413, #5409, #5355, #5348

Changelog: add #5381

Changelog: add #5412

Changelog: add #5386

Changelog: add #5250

CHangelog: add #5011

Changelog: add #5090

Changelog: add IDC fixes.

Changelog: add #5259

CHangelog: add #5186

Changelog: add #5236

Changelog: add #5284

Changelog: add #5366

Changelog: add #5382

Changelog: add #5396

Changelog: add #5405

Changelog: add #4897

Changelog: add #5289

Added a changelog entry about #5534.

Added a changelog entry about #5479.

Added a changelog entry about #5454.

Added a changelog entry about #5434.

Added a changelog entry about #5408.

Added a changelog entry about #5369.

Added a changelog entry about #5350.

Added a changelog entry about #5324.

Added a changelog entry about #5319.

Added a changelog entry about #5310.

Added a changelog entry about #5282.

Added a changelog entry about #5176.

Added a changelog entry about #3515.

Added a changelog entry about #1542.

Added a changelog entry about #5316.

Added a changelog entry about #3188.

Changelog: add #4987

Changelog: add #5270

Changelog: add #5225

Changelog: add #5507

Changelog: add #5432

Changelog: add #5473

Changelog: add #5392

Changelog: add #5222

Changelog: add #5457

Changelog: add #5423

Changelog: add #5332

Changelog: add #3853

Changelog: add #5237

Changelog: add #5307

Changelog: move up release headliner.

Changelog: add #5375

CHangelog: add #5496

Changelog: add #5528

Changelog: add #5537

Changelog: remove new Settings, they're punted to 4.5.

Changelog: move release headliner to the top.

Changelog: add testing list.

Changelog: add #4953

Changelog: add #5575

Changelog: add #5573

Changelog: add #5345

* Changelog: add #5168
@kraftbj kraftbj removed the [Status] Ready to Merge Go ahead, you can push that green button! label Oct 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Admin Page React-powered dashboard under the Jetpack menu [Pri] High [Status] Requires String Changes [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

6 participants