refactor(sftp-card): tsx, styled to scss, removed unused classNames, fixed checkbox padding, fixed cleaning up of the password #95474

nightnei · 2024-10-17T14:15:09Z

Related to #95435

Proposed Changes

With this PR we are fixing the gap for Enable SSH access for this site checkbox.
And also I decided to improve a few things:

Rewrite sftp-card component to tsx
Moved styled-css to .scss, for consistency with most of the component in Calypso
Removed a bunch of unused classNames
Fixed cleaning up of the password (thanks TSX for highlighting it).
I know that in Calypso we don't force to use tsx, but I advocate everybody to do it. 4-th point is good example, during rewriting it I faced a bug. It wasn't critical, but still.
4.1 We provided 3 arguments instead of 2 - the function expects siteId, username, but we provide siteId, currentUserId, username.
4.2 Cleaning up process was incorrect - we invoked destroy function immediately during render process, instead of on unmounting. We can see that intention was to return it, but by mistake it was immediately invoked.

Testing Instructions

Main issues

Have an Atomic site.
Look for it in wordpress.com/sites.
Click "Server Settings".
Click "Create credentials".
Turn on the "Enable SSH access for this site" toggle.
Assert that checbox and input are not stacked

BEFORE	AFTER

Refactor

Smoke test that everything looks and works the same way as before
TRy to reset password -> Open another tab (e.g. "Staging Site") -> Come back to "Server Settings" -> Assert that you see "Reset password" button

…fixed checkbox padding, fixed cleaning up of the password

github-actions · 2024-10-17T14:15:23Z

Calypso Live (direct link)

	https://calypso.live?image=registry.a8c.com/calypso/app:build-122364

Jetpack Cloud live (direct link)

	https://calypso.live?image=registry.a8c.com/calypso/app:build-122364&env=jetpack

Automattic for Agencies live (direct link)

	https://calypso.live?image=registry.a8c.com/calypso/app:build-122364&env=a8c-for-agencies

matticbot · 2024-10-17T14:23:21Z

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

Sections (~303 bytes removed 📉 [gzipped])

name          parsed_size           gzip_size
staging-site       -843 B  (-0.1%)     -303 B  (-0.1%)
hosting            -843 B  (-0.0%)     -303 B  (-0.1%)

Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to.

Legend

What is parsed and gzip size?

Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

fredrikekelund

This fixes the problem, and kudos for taking the extra step to refactor to TS 👍 I think we should consider dropping the use of connect, though (see my comment about that).

Also, could @matt-west please take a look at the margin between the Enable SSH toggle and the SSH snippet input and verify that it looks good from a design perspective? I believe this PR restores what we had previously, but it looks slightly cramped to me, so I just want to take the opportunity to tweak that if needed.

fredrikekelund · 2024-10-18T09:06:24Z

client/hosting/server-settings/components/sftp-card/index.tsx

+type SftpCardComponentProps = SftpCardProps & {
+	translate: LocalizeProps[ 'translate' ];
+	siteId: number | null;
+	siteSlug: string | null;
+	currentUserId: number | null;
+	username: string | null;
+	password: string | null;
+	siteHasSftpFeature: boolean;
+	siteHasSshFeature: boolean;
+	isSshAccessEnabled: boolean;
+	isLoadingSftpData: boolean;
+	createSftpUser: ( siteId: number | null, currentUserId: number | null ) => void;
+	requestSftpUsers: ( siteId: number | null ) => void;
+	resetSftpPassword: ( siteId: number | null, sshUsername: string | null ) => void;
+	requestSshAccess: ( siteId: number | null ) => void;
+	enableSshAccess: ( siteId: number | null ) => void;
+	disableSshAccess: ( siteId: number | null ) => void;
+	removePasswordFromState: ( siteId: number | null, username: string | null ) => void;
+};


The reason I typically avoid connect when converting JS files to TS is to avoid this redundancy. In other words, useSelector calls provide us with "free" types, whereas we have to manually respecify types when using connect.

I would advise we do the same in this case and employ useSelector and useDispatch instead of connect. It'll make the code more compact and future refactorings easier.

fredrikekelund · 2024-10-18T09:07:48Z

client/hosting/server-settings/components/sftp-card/style.scss

+.sftp-card__ssh-field .sftp-card__input {
+	margin-top: 5px;
+}


Nit, I would move this selector closer to the other .sftp-card__input selector.

fredrikekelund · 2024-10-18T09:11:45Z

client/hosting/server-settings/components/sftp-card/index.tsx

@@ -133,9 +126,19 @@ export const SftpCard = ( {
 				requestSshAccess( siteId );
 			}
 		}
-		return onDestroy();


This does indeed look like a mistake, even if it's hard to tell for sure at first glance. Did you verify this somehow, @nightnei?

Yes, as a result - we didn't clean it up during unmounting and it was possible to notice it for a short period of time when we did the second mounting (go back and forth between pages). Why for the short period of time - since we retrieve the list of users every time during mounting, so it wasn't a big issue. But for safety reasons - it's better to clean it up as soon as possible (it was what the author intended to do, but called onDestroy by mistake, and called it with wrong arguments - userId instead of username).

matt-west

Thanks for the ping @fredrikekelund!

I agree, we need some more spacing between the control and snippet below. 16px should be enough.

matticbot · 2024-10-21T14:25:47Z

This PR modifies the release build for the following Calypso Apps:

For info about this notification, see here: PCYsg-OT6-p2

notifications
wpcom-block-editor

To test WordPress.com changes, run install-plugin.sh $pluginSlug nightnei/refactorSftpCard on your sandbox.

fredrikekelund

Dropping connect broke the tests, so I had to make some tweaks to get them working again. LGTM now, though 👍

fredrikekelund · 2024-10-21T15:21:28Z

client/hosting/server-settings/components/sftp-card/index.tsx

+	const { username, password } = useSelector( ( state ) => {
+		let username;
+		let password;
+
+		const users = getAtomicHostingSftpUsers( state, siteId );
+
+		if ( ! disabled && users !== null ) {
+			if ( users.length ) {
+				// Pick first user. Rest of users will be handled in next phases.
+				username = users[ 0 ].username;
+				password = users[ 0 ].password;
+			} else {
+				// No SFTP user created yet.
+				username = null;
+				password = null;
+			}
+		}
+
+		return { username, password };
+	} );


I copied the old logic exactly because this component relies on a difference between null and undefined values here (something I learned while fixing the tests).

Definitely not an ideal solution to begin with, but the scope of this PR is already pretty large, so if we want to fix that, we should do it in another PR (I don't think it warrants fixing right now btw)

refactor(sftp-card): tsx, styled to scss, removed unused classNames, …

a5a212f

…fixed checkbox padding, fixed cleaning up of the password

nightnei linked an issue Oct 17, 2024 that may be closed by this pull request

Server Settings: Missing padding between SSH toggle and text box #95435

Closed

nightnei requested a review from fredrikekelund October 17, 2024 14:43

matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Oct 17, 2024

nightnei requested a review from wongasy October 17, 2024 14:43

fredrikekelund reviewed Oct 18, 2024

View reviewed changes

fredrikekelund requested a review from matt-west October 18, 2024 09:15

matt-west reviewed Oct 18, 2024

View reviewed changes

nightnei and others added 4 commits October 18, 2024 12:34

chore: align spaces

e655e6d

chore: remove connect

ca2dba0

Small tweaks

8267bf7

Merge branch 'trunk' into nightnei/refactorSftpCard

a2f35ed

Fix tests

461a0fe

fredrikekelund approved these changes Oct 21, 2024

View reviewed changes

Type fix

93af05d

fredrikekelund merged commit 791ec29 into trunk Oct 22, 2024
11 checks passed

fredrikekelund deleted the nightnei/refactorSftpCard branch October 22, 2024 07:48

github-actions bot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Oct 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(sftp-card): tsx, styled to scss, removed unused classNames, fixed checkbox padding, fixed cleaning up of the password #95474

refactor(sftp-card): tsx, styled to scss, removed unused classNames, fixed checkbox padding, fixed cleaning up of the password #95474

nightnei commented Oct 17, 2024 •

edited

Loading

github-actions bot commented Oct 17, 2024 •

edited by matticbot

Loading

matticbot commented Oct 17, 2024 •

edited

Loading

fredrikekelund left a comment

fredrikekelund Oct 18, 2024

fredrikekelund Oct 18, 2024

fredrikekelund Oct 18, 2024

nightnei Oct 18, 2024 •

edited

Loading

matt-west left a comment

matticbot commented Oct 21, 2024 •

edited

Loading

fredrikekelund left a comment

fredrikekelund Oct 21, 2024

refactor(sftp-card): tsx, styled to scss, removed unused classNames, fixed checkbox padding, fixed cleaning up of the password #95474

refactor(sftp-card): tsx, styled to scss, removed unused classNames, fixed checkbox padding, fixed cleaning up of the password #95474

Conversation

nightnei commented Oct 17, 2024 • edited Loading

Proposed Changes

Testing Instructions

Main issues

Refactor

github-actions bot commented Oct 17, 2024 • edited by matticbot Loading

matticbot commented Oct 17, 2024 • edited Loading

fredrikekelund left a comment

Choose a reason for hiding this comment

fredrikekelund Oct 18, 2024

Choose a reason for hiding this comment

fredrikekelund Oct 18, 2024

Choose a reason for hiding this comment

fredrikekelund Oct 18, 2024

Choose a reason for hiding this comment

nightnei Oct 18, 2024 • edited Loading

Choose a reason for hiding this comment

matt-west left a comment

Choose a reason for hiding this comment

matticbot commented Oct 21, 2024 • edited Loading

fredrikekelund left a comment

Choose a reason for hiding this comment

fredrikekelund Oct 21, 2024

Choose a reason for hiding this comment

nightnei commented Oct 17, 2024 •

edited

Loading

github-actions bot commented Oct 17, 2024 •

edited by matticbot

Loading

matticbot commented Oct 17, 2024 •

edited

Loading

nightnei Oct 18, 2024 •

edited

Loading

matticbot commented Oct 21, 2024 •

edited

Loading