Skip to content

Conversation

eliorivero
Copy link
Contributor

Fixes #6226

Changes proposed in this Pull Request:

  • add link in Monitor card in At a Glance pointing to Security settings in Calypso where user will be able to manage notifications.

This is how it looks in this PR, which is what @MichaelArestad suggested in #6226

captura de pantalla 2017-02-16 a las 19 32 28

captura de pantalla 2017-02-16 a las 19 32 39

Since this card also has a toggle and there were no instructions about what to do with the toggle, I assumed we should leave it and tested adding a cog icon

captura de pantalla 2017-02-16 a las 19 37 18

This is not in this PR but it's trivial to implement it.

If we keep this, it would be good to have an id in the card Jetpack Monitor in Calypso so we can add it to the link and scroll the view straight to that card.

captura de pantalla 2017-02-16 a las 20 01 10

  • removes props siteAdminUrl and siteRawUrl that were passed in cascade (and unused in intermediate components) and instead fetches them in DashItem where they're used.

  • updates test to reflect that Monitor now has a button in addition to the toggle

Testing instructions:

  • should not be visible in dev mode
  • should not be visible when card is toggled off
  • should be visible when card is toggled on and point to https://wordpress.com/settings/security/%site%

@eliorivero eliorivero added Admin Page React-powered dashboard under the Jetpack menu [Status] Needs Design Review Design has been added. Needs a review! [Status] Needs Review This PR is ready for review. [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it labels Feb 16, 2017
@eliorivero eliorivero added this to the Settings UI milestone Feb 16, 2017
@eliorivero eliorivero self-assigned this Feb 16, 2017
@beaulebens
Copy link
Member

In your screenshot above, you can see that there's a "fade out" image that's showing (next to the toggle), which shouldn't be there any more (FWIW, I see it on all "disabled" cards on the dashboard):

I see the same problem on a dev-mode site:

screen shot 2017-02-19 at 7 16 40 pm

I'll defer to @MichaelArestad on whether to use the button or a cog icon.

I think the toggle was actually intended to go away, since the idea was that Monitor can run without much impact (always), and if you don't want to receive notifications then you can just turn those off (thus the settings link).

@MichaelArestad
Copy link
Contributor

I would remove the toggle and just use the button.

@beaulebens created an issue: #6447

@eliorivero
Copy link
Contributor Author

eliorivero commented Feb 20, 2017

captura de pantalla 2017-02-20 a las 17 46 41

Updated to display only the button.

…play the Settings button when it's active. Update GUI tests.
@eliorivero eliorivero force-pushed the add/monitor-link-calypso-security branch from aaf8325 to 85fb805 Compare February 20, 2017 20:46
@MichaelArestad
Copy link
Contributor

LGTM

@MichaelArestad MichaelArestad removed the [Status] Needs Design Review Design has been added. Needs a review! label Feb 20, 2017
@dereksmart dereksmart merged commit 6b23c8c into feature/settings-overhaul Feb 21, 2017
@dereksmart dereksmart deleted the add/monitor-link-calypso-security branch February 21, 2017 16:19
@dereksmart dereksmart removed the [Status] Needs Review This PR is ready for review. label Feb 21, 2017
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 [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants