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

Hosting Panel: Update UI for cards and add clipboard button #36864

Merged
merged 6 commits into from
Oct 22, 2019

Conversation

jeraldjuice
Copy link

@jeraldjuice jeraldjuice commented Oct 17, 2019

Summary

Per design feedback, this PR updates the UI for the cards in the Hosting panel, as well as adding a clipboard button in order to copy user login information.


Screenshots

When no SFTP user is available
Screen Shot 2019-10-17 at 12 12 59 PM

When an SFTP user is available
Screen Shot 2019-10-17 at 12 12 38 PM

When an SFTP user is created or the password is reset
Screen Shot 2019-10-17 at 12 12 47 PM


Testing

  • Spin up a local Calypso development environment.
  • Apply D34243-code to your sandbox.
  • Go to http://calypso.localhost:3000/hosting/your.atomic.site.
  • Ensure that UI states reflected in the screenshots are visible.
  • Ensure that 'Copy' buttons work as expected.

@jeraldjuice jeraldjuice 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, 2019
@jeraldjuice jeraldjuice added this to the Hosting Section - Phase 1 milestone Oct 17, 2019
@jeraldjuice jeraldjuice requested review from shaunandrews and a team October 17, 2019 16:23
@jeraldjuice jeraldjuice self-assigned this Oct 17, 2019
@matticbot
Copy link
Contributor

@matticbot
Copy link
Contributor

matticbot commented Oct 17, 2019

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

Webpack Runtime (~75 bytes added 📈 [gzipped])

name      parsed_size           gzip_size
manifest       +231 B  (+0.1%)      +75 B  (+0.2%)

Webpack runtime for loading modules. It is included in the HTML page as an inline script. Is downloaded and parsed every time the app is loaded.

App Entrypoints (~3 bytes added 📈 [gzipped])

name        parsed_size           gzip_size
entry-main        +23 B  (+0.0%)       +3 B  (+0.0%)

Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used.

Sections (~3415 bytes added 📈 [gzipped])

name            parsed_size            gzip_size
hosting            +12687 B  (+12.5%)    +3399 B  (+12.1%)
wp-super-cache        +18 B   (+0.0%)       +8 B   (+0.0%)
plans                 +18 B   (+0.0%)       +5 B   (+0.0%)
marketing             +18 B   (+0.0%)       +3 B   (+0.0%)

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.

@gwwar
Copy link
Contributor

gwwar commented Oct 17, 2019

Total aside and this can go into a follow up PR, but @shaunandrews did we have any suggested error states? I suspect a global error notice https://wpcalypso.wordpress.com/devdocs/design/global-notices may make sense, but let us know if you'd prefer something inline.

@gwwar
Copy link
Contributor

gwwar commented Oct 17, 2019

Looks like the username can still break out of the card at certain breakpoints. I think we can add a max-width with a trailing gradient to hide the extra text. Open to whatever suggestions that @shaunandrews has though if we stopped using that pattern.

Screen Shot 2019-10-17 at 11 14 31 AM

{ password && (
<div className="sftp-card__callout-box">
<p>{ translate( 'Your new password for your sftp user is:' ) }</p>
{ errorCode === 404 ? (
Copy link
Contributor

Choose a reason for hiding this comment

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

For folks 👀 We can further tidy the data pattern in a different PR. Basically we have a default retry policy that will, retry 3 times on any GET requests for a non 2xx status code. In this case the 404 is a clear answer, so we either have a choice of updating the API response or modifying client behavior.

Note that we need to also still need to handle non 404 cases (5xx)

@gwwar
Copy link
Contributor

gwwar commented Oct 17, 2019

✅ copy buttons work as expected, and overall code changes look good. I'll wait for @shaunandrews to chime in but this has my tentative approval otherwise.

We can follow up with more error cases in another PR.

@shaunandrews
Copy link
Contributor

did we have any suggested error states

Do we know what errors might occur?

@shaunandrews
Copy link
Contributor

I think we can add a max-width with a trailing gradient to hide the extra text.

That could work, though I'd also be OK with just truncating the text with an ellipsis at the end.

@shaunandrews
Copy link
Contributor

just truncating the text with an ellipsis at the end.

Actually, that might hinder copy/paste. Hmm. Maybe we force the username to wrap?

@gwwar
Copy link
Contributor

gwwar commented Oct 17, 2019

Do we know what errors might occur?

  • We fail to create an SSH user.
  • The Password Reset Fails
  • Database Access Fails for some reason.

@glendaviesnz
Copy link
Contributor

glendaviesnz commented Oct 21, 2019

@glendaviesnz
Copy link
Contributor

glendaviesnz commented Oct 22, 2019

Actually, that might hinder copy/paste. Hmm. Maybe we force the username to wrap?

I just tested, and applying

    overflow: hidden;
    text-overflow: ellipsis;
    white-space: nowrap;
    width: 95%;

to the username paragraph doesn't affect copy and paste - it still copies the full username - this is a css only change which the copy button doesn't care about.

The width setting above isn't correct though, this still pushes out at some breakpoints - haven't managed to work out why yet - thought it might require a calc() to get a pixel width but that didn't work either.

@jeraldjuice jeraldjuice force-pushed the update/hosting-panel-ui-tweaks branch from 9c46b90 to 4686d44 Compare October 22, 2019 01:22
@jeraldjuice
Copy link
Author

Force-pushed to handle some merge conflicts.

@jeraldjuice
Copy link
Author

As @glendaviesnz mentioned, css text wrapping won't affect the copy button. It will, however, make it a bit more difficult to manually copy the text/view it. Here's how it looks with the ellipses wrapping the password/username:

Screen Shot 2019-10-21 at 9 50 58 PM

@shaunandrews
Copy link
Contributor

did we have any suggested error states? I suspect a global error notice may make sense, but let us know if you'd prefer something inline.

I think a global notice could work if we only have very short messages without any related actions. An inline notice might be slightly more emphatic — and preferred if we end up with larger error messages or related actions.

css text wrapping won't affect the copy button. It will, however, make it a bit more difficult to manually copy the text/view it.

Yea, sorry, I was referring to old fashioned manual copying. I think since we have the button, its not a major concern. It also begs the question of if we even need to show the username or password and force people to use the copy button.

@jeraldjuice
Copy link
Author

@shaunandrews I'm going to merge this in and open up a new PR for the error states. We can also iterate on the password/username copying there, too.

@jeraldjuice jeraldjuice merged commit 11dcffe into master Oct 22, 2019
@jeraldjuice jeraldjuice deleted the update/hosting-panel-ui-tweaks branch October 22, 2019 17:02
@matticbot matticbot 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, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants