Skip to content

Clean AAG/Dashboard cards. #7188

Merged
eliorivero merged 7 commits intomasterfrom
update/dashboard-use-settings-actions
May 23, 2017
Merged

Clean AAG/Dashboard cards. #7188
eliorivero merged 7 commits intomasterfrom
update/dashboard-use-settings-actions

Conversation

@dereksmart
Copy link
Copy Markdown
Contributor

@dereksmart dereksmart commented May 16, 2017

This is not a functional change. It is a refactor of all of the Dashboard cards to:

  • Use the actions from state/settings/, rather than the old (deprecated) state/modules/ actions.
  • Clean up all eslint warnings, including registering es6 class components. (Cleaned up around 60 warnings)
  • Make this more dry. We're now passing down the settings actions through so that each dash card does not need to register it's own.
  • Remove unused code. For example, we were no longer using the DashSiteVerify component, so I removed it completely.

We should be moving to completely deprecate the use of state/modules/ actions, like activateModule, deactivateModule, etc... and instead module activation should be done through state/settings/. This is one step in that direction, as there are still other places that use the old actions. Submitting this now so that it doesn't get too crazy.

TO TEST:

  • Do everything imaginable in the Dashboard area.
  • Do it again, and in a different state (different plan, etc...)
  • Activate/deactivate ALL of the modules listed there
  • Test the connection stuff
  • Test it in non-admin mode

@dereksmart dereksmart added [Status] Needs Review This PR is ready for review. Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Type] Janitorial labels May 16, 2017
@dereksmart dereksmart requested review from eliorivero and oskosk May 16, 2017 15:47
@eliorivero
Copy link
Copy Markdown
Contributor

@oskosk
Copy link
Copy Markdown
Contributor

oskosk commented May 16, 2017

Such a diff!

import { ModuleSettingsForm as moduleSettingsForm } from 'components/module-settings/module-settings-form';
import DashSectionHeader from 'components/dash-section-header';
import DashStats from './stats';
import DashStats from './stats/index.jsx';
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not quite clear to me why we need the extra /index.jsx here

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We technically don't but my text editor was complaining about it not being there /shrug

isModuleActivated: ( module_name ) => _isModuleActivated( state, module_name ),
getSitePlan: () => getSitePlan( state )
getAkismetData: getAkismetData( state ),
getSitePlan: getSitePlan( state )
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

These two prop names are confusing now that they're no longer functions. akimestData and getSitePlan would be clearer

@dereksmart
Copy link
Copy Markdown
Contributor Author

@eliorivero

there are some eslint errors warnings at
client/state/at-a-glance/index.js
client/at-a-glance/backups.jsx
client/at-a-glance/connections.jsx

Fixed these. The warnings coming from at-a-glance/connections.jsx was about us using the wrong gridicon size, jsx-gridicon-size, so I just disabled the rule rather than changing the design we already had.

@dereksmart dereksmart force-pushed the update/dashboard-use-settings-actions branch from 917bfd5 to 7ca0aed Compare May 16, 2017 20:18
Copy link
Copy Markdown
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.

Tests are ok and as well as eslint for at-a-glance/ files.

captura de pantalla 2017-05-16 a las 17 58 35

However, user connection is broken for me.

@dereksmart
Copy link
Copy Markdown
Contributor Author

However, user connection is broken for me.

@eliorivero I can't reproduce. Can you see if this happens for you in other branches as well? Or after you cycle the connection? Or can you look at what props are being passed to that component? Thanks!

@oskosk
Copy link
Copy Markdown
Contributor

oskosk commented May 19, 2017

LGTM, couldn't reproduce the error with the connection dashboard item

@dereksmart dereksmart added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels May 19, 2017
@eliorivero
Copy link
Copy Markdown
Contributor

Connection error is gone now. Not sure why I was experiencing this before. It's ok so will merge.

@eliorivero eliorivero merged commit a27b570 into master May 23, 2017
@eliorivero eliorivero deleted the update/dashboard-use-settings-actions branch May 23, 2017 22:22
@eliorivero eliorivero removed the [Status] Ready to Merge Go ahead, you can push that green button! label May 23, 2017
@jeherve jeherve added [Status] Needs Changelog [Status] Needs Testing We need to add this change to the testing call for this month's release and removed [Status] Needs Changelog labels May 24, 2017
jeherve added a commit that referenced this pull request May 29, 2017
eliorivero pushed a commit that referenced this pull request May 30, 2017
* Changelog: first pass at a changelog for 5.0

* Changelog: delete 4.9 testing list.

* Changelog: update minimum WP version to match ver. in jetpack.php

Fixes #7158

* Changelog: add #6051

* Changelog: add #6753

* Changelog: add #6928

* Changelog: add #6964

* Changelog: add #7014

* Changelog: add #7057

* Changelog: add #7060

* Changelog: add #7068

* Changelog: add #7070

* Changelog: add #7072

* Changelog: add #7071

* Changelog: add release date and post shortlink.

* Changelog: add #7094

* Changelog: add #7100

* Changelog: add #7108

* Changelog: add #7113

* Changelog: add #7123

* Changelog: add #7135

* Changelog: add #7143

* Changelog: add #7151

* Changelog: add #6996

* Changelog: add #7105

* Changelog: add #7132

* Changelog: add #7166

* Changelog: fix typo in 4.9 changelog.

* Changelog: remove older releases' changelogs.

@see p1HpG7-42e-p2

* Changelog: add #7090

* Changelog: add #7095

* Changelog: add #7112

* Changelog: add #7115

* Changelog: add #7122

* Changelog: add #7137

* Changelog: add #7138

* Changelog: add #7140

* Changelog: add #7154

* Changelog: add ##7155

* Changelog: add #7163

* Changelog: add #7167

* Changelog: add #7171

* Changelog: add #7180

* Changelog: add #7181

* Changelog: add #7183

* Changelog: add #7184

* Changelog: add #7189

* Changelog: add #7191

* Changelog: add #7193

* Changelog: add #7198

* Changelog: add #7200

* Changelog: add #7209

* Changelog: add #7212

* Testing list: add instructions for #7115

* Changelog: add #7188

* Changelog: add #7205

* Changelog: add #7225

* Changelog: add #6872

* Changelog: add #7107

* Changelog: add #7118

* Changelog: add #7142

* Changelog: add #7170

* Changelog: add #7210

* Changelog: add #7218

* Changelog: add #7232

* Changelog: add #7211

* Changelog: add #7213

* Changelog: add #7229

* Changelog: add #7230

* Changelog: add #7214

* Draft changelog for 5.0

* Changelog updates: 2nd pass at a clearer changelog.

- Fix typos.
- Use consistent tense and tone across all changelog.
- Remove unclear items.

* Changelog: add #7026

* Changelog: add #7058

* Changelog: add #7125

* Changelog: add #7249

* Changelog: add #7185

* add mentions of image widget migration

* Changelog: add info about new output for CLI command.

* Changelog: add WP version number matching the new Image Widget.
@jeherve jeherve removed the [Status] Needs Testing We need to add this change to the testing call for this month's release label Apr 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Type] Janitorial

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants