Site Settings: create a Back navigation button and add it to the JP Disconnect Site #17691

AnnaMag · 2017-09-01T07:22:32Z

Purpose: add an option of site Return (Back button) to the Jetpack Disconnect Flow.

Included in the patch:

create a NavigationBackButton component, where the destination route is passed as a prop.
add this component to the Jetpack Disconnect Site to have an option of Return to the starting point of the Disconnect flow (Manage Connection link in /settings/general).

To test:

Checkout this branch, or use calypso.live
https://calypso.live/?branch=update/site-settings-disconnect-site-add-return
Go to settings/disconnect-site/:site

For Jetpack and non-Atomic sites:

verify that the Back button is visible in the left top corner and redirects to /settings/manage-connection/:site, where :site is the Jetpack site.

All other sites: have no access = no site visibility

Note: please ignore the site settings specific Placeholder used here. This patch does not deal with adjusting it to the Disconnect Flow ( coming up).

Visuals:

keoshi

Looking good though slightly out of place — added a few comments inline.

keoshi · 2017-09-01T11:56:57Z

client/my-sites/site-settings/render-return-button/style.scss

+		left: 600px;
+		bottom: 200px;
+
+	.button.is-compact.is-borderless {


.render-return-button__container is being applied directly to the button itself, so this property isn't being applied and should be moved to the same level and the first class.

keoshi · 2017-09-01T12:01:53Z

client/my-sites/site-settings/disconnect-site/index.jsx

 	render() {
 		const { translate } = this.props;

 		return (
 			<Main className="disconnect-site site-settings">
 				<DocumentHead title={ translate( 'Site Settings' ) } />
+				<ReturnToPreviousPage onBackClick={ this.handleClickBack } { ...this.props } />


The button should be on the very top left-hand corner of the view area, so maybe we need to pop it outside of the Main area and create an outer div.

Same level as the '<- Switch Site', correct?

Yes, kind of like this:

We could even make the case that it's better to drop the sidebar altogether for this survey. What do you think @rickybanister ?

I agree with that. The sidebar only really provides a distraction here. Full screen will help us accomplish our goal better and will help us improve the mobile view more.

keoshi · 2017-09-29T12:21:57Z

client/my-sites/site-settings/disconnect-site/style.scss

@@ -32,3 +30,9 @@
 			text-align: center;
 		}
 }
+
+.disconnect-site__back-button {


By using percentages here, the button will “float” around depending on the screen's resolution. Instead, we can just use the same styles as .reader-full-post__back-container (ref:

wp-calypso/client/blocks/reader-full-post/style.scss

Lines 37 to 60 in 64d9a90

.reader-full-post__back-container {

margin: 0;

position: fixed;

top: 47px;

left: 0;

z-index: z-index( '.masterbar', '.reader-back' );

.button.is-compact.is-borderless {

padding: 18px 40px 18px 20px;

@include breakpoint( "<660px" ) {

padding: 20px 8px 16px;

}

}

@include breakpoint( "<660px" ) {

top: 0;

left: 0;

right: 0;

height: 46px;

background: $white;

border-bottom: 1px solid $gray-light;

}

}

), which in this scenario would be:

.disconnect-site__back-button { margin: 0; position: fixed; top: 47px; left: 0; z-index: z-index( '.masterbar', '.reader-back' ); .button.is-compact.is-borderless { padding: 18px 40px 18px 20px; @include breakpoint( "<660px" ) { padding: 20px 8px 16px; } } @include breakpoint( "<660px" ) { top: 0; left: 0; right: 0; height: 46px; background: $white; border-bottom: 1px solid $gray-light; } }

Makes sense, thanks @keoshi.

I also moved the button specific styling to the component's style file. IMHO the idea would be to define button's container styling when using it (e.g. its positioning ), and not worry about the looks of it.

seear · 2017-09-29T15:48:17Z

client/my-sites/site-settings/navigation-back-button/return-previous-page/index.jsx

@@ -0,0 +1,33 @@
+/** @format */


Should this file be removed?

yes, ofc. Bad copy. Thanks!

seear · 2017-09-29T15:50:09Z

client/my-sites/site-settings/navigation-back-button/index.jsx

@@ -0,0 +1,33 @@
+/** @format */


This is a generic component, not restricted to use with the site settings, so I think it may make sense for it to live at https://github.com/Automattic/wp-calypso/tree/master/client/components.

seear · 2017-09-29T15:54:31Z

client/my-sites/site-settings/disconnect-site/index.jsx

-			</Main>
+			<div>
+				<span className="disconnect-site__back-button">
+					<NavigationBackButton redirectRoute={ this.getRoute() } { ...this.props } />


Seems passing ...this.props is not necessary.

@seear, this is to pass e.g. translate as a prop, since the component is not connected. Is there a better way to go about it?

Ah, ok! The call to localize() wraps a component with the localize higher-order-component, which has some useful props, one of which is translate, meaning we don't need to pass it down.

seear · 2017-09-29T15:57:48Z

client/my-sites/site-settings/navigation-back-button/README.md

+
+#Props:
+
+- `getRoute()` is a function returning redirection route loaded in the `onClick`


now takes redirectRoute as a string.

Yes, true. Thanks!

seear · 2017-09-29T16:00:04Z

@AnnaMag This is looking good. I left a few comments, the major one to consider being whether to move this to the shared /components folder, which on balance I think we should.

Name was changed to better reflect the generic functionality of the component.

…omponent Previously the positioning of the button was expressed in %, which made affected its positioning depending on the screen resolution. In addition to fixing the latter, the style properties referring to the button itself are placed in component specific style.css file. Therefore, to use it it is only required to style the container of the button component (e.g. its positioning). To make it clear, the `className` of the button container was changed to reflect that.

AnnaMag · 2017-10-01T22:55:25Z

Status: included the feedback + improved styling.
To-do: move the button to /components

tyxla

Really solid work here @AnnaMag!

Left some drive-by comments to add to what @seear found already. Nothing major really 😉

tyxla · 2017-10-03T10:33:03Z

client/my-sites/site-settings/disconnect-site/index.jsx

+	getRoute() {
+		const { siteSlug } = this.props;
+
+		if ( siteSlug ) {


This is totally unnecessary with the current implementation (because we're displaying a placeholder if sites are not loaded), but just asking: is it intentional that we don't return anything if siteSlug is not there yet? Do we want to safeguard and return /settings/manage-connection there in that case?

tyxla · 2017-10-03T10:35:07Z

client/my-sites/site-settings/disconnect-site/index.jsx

-			</Main>
+			<div>
+				<span className="disconnect-site__back-button-container">
+					<NavigationBackButton redirectRoute={ this.getRoute() } { ...this.props } />


Do we really want to pass all props down to <NavigationBackButton />? It's always more readable and sustainable to be specific about which props you're passing down, rather than just passing them all. And that's even more valid in this case - I don't see any additional props necessary there, so maybe remove { ...this.props } completely?

tyxla · 2017-10-03T10:38:14Z

client/my-sites/site-settings/navigation-back-button/README.md

+import NavigationBackButton from 'my-sites/site-settings/navigation-back-button';
+...
+
+getRoute() {


This one is probably not necessary. Since we try to keep examples as simple as possible, we can just include a dummy path here instead of a method.

tyxla · 2017-10-03T10:38:48Z

client/my-sites/site-settings/navigation-back-button/README.md

+	... // return a route
+}
+
+<span className="....">


Would you like to place this within a render() method, just so it's clear where one's supposed to be calling this one? Also, maybe you want to remove { ...this.props } from the example.

tyxla · 2017-10-03T10:40:46Z

client/my-sites/site-settings/navigation-back-button/README.md

+
+- `redirectRoute` -- destination redirect route (string)
+
+##Exemplary use


I'd say we need only the section that demonstrates how NavigationBackButton works, without the one that demonstrates how getRoute works - to me that one is unnecessary in the context of the back button.

tyxla · 2017-10-03T10:41:20Z

client/my-sites/site-settings/navigation-back-button/README.md

+}
+
+```
+- `props` -- used to pass `translate` function to the component


We usually don't pass translate down to other components; we rather prefer wrapping the component that needs to call translate with the localize HoC. You actually did this already, so you might want to remove this line from the readme.

tyxla · 2017-10-03T10:43:59Z

client/my-sites/site-settings/navigation-back-button/index.jsx

+import Button from 'components/button';
+
+const NavigationBackButton = ( { redirectRoute, translate } ) => {
+	const handleClick = () => {


How about using a Component or a PureComponent instead of a stateless functional component here? The problem with this approach is that it will generate a new handleClick handler on every render.

Let's avoid page routing altogether when there's an href prop we can use: https://github.com/Automattic/wp-calypso/blob/master/client/components/button/README.md#props

ockham · 2017-10-09T14:56:57Z

Okay, this is ready for another round of reviews.

ockham · 2017-10-09T15:28:20Z

Oh, turns out we might not need this after all: #17696 (comment)

ockham · 2017-10-11T21:36:49Z

Closing, since #18749 uses components/wizard/navigation-link instead.

matticbot added OSS Citizen [Size] M Medium sized issue labels Sep 1, 2017

AnnaMag added [Feature] Site Settings All other general site settings. [Status] In Progress [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR labels Sep 1, 2017

AnnaMag requested a review from keoshi September 1, 2017 07:23

AnnaMag mentioned this pull request Sep 1, 2017

Site Settings/Disconnect Site: add Survey details to the first screen of the JP Disconnect Flow #17692

Closed

AnnaMag force-pushed the update/site-settings-disconnect-site-add-return branch from b3e8ca8 to 766c48f Compare September 1, 2017 11:17

matticbot added [Size] L Large sized issue and removed [Size] M Medium sized issue labels Sep 1, 2017

keoshi reviewed Sep 1, 2017

View reviewed changes

AnnaMag mentioned this pull request Sep 1, 2017

Site Settings: add Survey Q1 to the JP Disconnect UX #17696

Closed

AnnaMag force-pushed the add/site-settings-disconnect-site branch 3 times, most recently from 71ab31d to f148713 Compare September 2, 2017 21:45

AnnaMag force-pushed the update/site-settings-disconnect-site-add-return branch from 766c48f to 37effcf Compare September 3, 2017 16:52

matticbot added [Size] M Medium sized issue and removed [Size] L Large sized issue labels Sep 3, 2017

AnnaMag force-pushed the update/site-settings-disconnect-site-add-return branch from 37effcf to feef6a6 Compare September 5, 2017 10:13

matticbot added [Size] L Large sized issue and removed [Size] M Medium sized issue labels Sep 5, 2017

AnnaMag force-pushed the update/site-settings-disconnect-site-add-return branch 3 times, most recently from d25df7b to 0b2f887 Compare September 5, 2017 11:20

AnnaMag force-pushed the add/site-settings-disconnect-site branch 2 times, most recently from f67fca9 to aec29f3 Compare September 7, 2017 08:43

AnnaMag force-pushed the update/site-settings-disconnect-site-add-return branch 2 times, most recently from 6f512ad to 2c69a4f Compare September 7, 2017 20:18

AnnaMag self-assigned this Sep 8, 2017

AnnaMag force-pushed the add/site-settings-disconnect-site branch from aec29f3 to 19aa1dc Compare September 13, 2017 13:42

AnnaMag added the [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR label Sep 29, 2017

AnnaMag force-pushed the update/site-settings-disconnect-site-add-return branch 3 times, most recently from 6eda017 to 3818380 Compare September 29, 2017 11:53

keoshi reviewed Sep 29, 2017

View reviewed changes

keoshi removed the [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR label Sep 29, 2017

seear reviewed Sep 29, 2017

View reviewed changes

AnnaMag force-pushed the update/site-settings-disconnect-site-add-return branch 2 times, most recently from 116645b to 255a862 Compare October 1, 2017 12:24

AnnaMag added 2 commits October 1, 2017 13:25

Rename ReturnPreviousPage to NavigationBackButton

255a862

Name was changed to better reflect the generic functionality of the component.

AnnaMag force-pushed the update/site-settings-disconnect-site-add-return branch from ac89842 to 69303cc Compare October 1, 2017 22:52

NavigationBackButton: correct props section in the README.md

1b0408e

tyxla reviewed Oct 3, 2017

View reviewed changes

ockham added 6 commits October 9, 2017 16:19

Address README feedback

a062e6e

Drop wholesale props passing

23243ec

Turn DisconnectSite into a functional component

7ca995c

Use href instead of onClick

4f37793

Move to client/components

92c8453

Add devdocs example

3a757a6

ockham closed this Oct 11, 2017

ockham deleted the update/site-settings-disconnect-site-add-return branch October 11, 2017 21:36

matticbot removed [Status] In Progress [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Oct 11, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Site Settings: create a Back navigation button and add it to the JP Disconnect Site #17691

Site Settings: create a Back navigation button and add it to the JP Disconnect Site #17691

AnnaMag commented Sep 1, 2017 •

edited

Loading

keoshi left a comment

keoshi Sep 1, 2017

keoshi Sep 1, 2017

AnnaMag Sep 1, 2017

keoshi Sep 1, 2017

rickybanister Sep 20, 2017

keoshi Sep 29, 2017

AnnaMag Sep 29, 2017

AnnaMag Oct 1, 2017

seear Sep 29, 2017

AnnaMag Sep 30, 2017

seear Sep 29, 2017

seear Sep 29, 2017

AnnaMag Oct 1, 2017 •

edited

Loading

seear Oct 2, 2017

seear Sep 29, 2017

AnnaMag Sep 30, 2017

seear commented Sep 29, 2017

AnnaMag commented Oct 1, 2017 •

edited

Loading

tyxla left a comment

tyxla Oct 3, 2017

tyxla Oct 3, 2017

tyxla Oct 3, 2017

tyxla Oct 3, 2017

tyxla Oct 3, 2017

tyxla Oct 3, 2017

tyxla Oct 3, 2017

ockham Oct 9, 2017

ockham commented Oct 9, 2017

ockham commented Oct 9, 2017

ockham commented Oct 11, 2017

	.reader-full-post__back-container {
	margin: 0;
	position: fixed;
	top: 47px;
	left: 0;
	z-index: z-index( '.masterbar', '.reader-back' );

	.button.is-compact.is-borderless {
	padding: 18px 40px 18px 20px;

	@include breakpoint( "<660px" ) {
	padding: 20px 8px 16px;
	}
	}

	@include breakpoint( "<660px" ) {
	top: 0;
	left: 0;
	right: 0;
	height: 46px;
	background: $white;
	border-bottom: 1px solid $gray-light;
	}
	}


		#Props:

		- `getRoute()` is a function returning redirection route loaded in the `onClick`


		- `redirectRoute` -- destination redirect route (string)

		##Exemplary use

Site Settings: create a Back navigation button and add it to the JP Disconnect Site #17691

Site Settings: create a Back navigation button and add it to the JP Disconnect Site #17691

Conversation

AnnaMag commented Sep 1, 2017 • edited Loading

keoshi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnnaMag Oct 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seear commented Sep 29, 2017

AnnaMag commented Oct 1, 2017 • edited Loading

tyxla left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ockham commented Oct 9, 2017

ockham commented Oct 9, 2017

ockham commented Oct 11, 2017

AnnaMag commented Sep 1, 2017 •

edited

Loading

AnnaMag Oct 1, 2017 •

edited

Loading

AnnaMag commented Oct 1, 2017 •

edited

Loading