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

Sites Management Page: Visually distinguish 'redirect' sites #66750

Merged
merged 3 commits into from
Aug 19, 2022

Conversation

danielbachhuber
Copy link
Contributor

Fixes #66746

Proposed Changes

Visually distinguishes 'redirect' sites in the List view and Grid view, and uses site.options.unmapped_url instead of site.URL for redirect sites:

image

image

Testing Instructions

  1. Create a new Free site.
  2. Add the Site Redirect Add-On to the new Free site: https://wordpress.com/domains/add/site-redirect/
  3. Navigate to /sites-dashboard.
  4. Verify the Free site with redirect appears as expected in both List view and Grid view.

@danielbachhuber danielbachhuber added [Type] Task Sites Management Page i1 Reviewer Can Merge PR author indicates the reviewer is free to merge/deploy if they want to own the change. labels Aug 19, 2022
@danielbachhuber danielbachhuber requested a review from a team August 19, 2022 11:53
@danielbachhuber danielbachhuber self-assigned this Aug 19, 2022
@matticbot matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Aug 19, 2022
@github-actions
Copy link

github-actions bot commented Aug 19, 2022

@matticbot
Copy link
Contributor

matticbot commented Aug 19, 2022

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

Sections (~101 bytes added 📈 [gzipped])

name             parsed_size           gzip_size
sites-dashboard       +618 B  (+0.3%)     +101 B  (+0.2%)

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.

@zaguiini
Copy link
Contributor

Works well 👍

One thing I noticed is that the site switcher displays both the site status pill and the redirect pill:

image

While the SMD only displays the redirect status:

image

But I don't think it's incorrect. The site status doesn't matter if it's a redirect because the redirect takes effect immediately, so it has precedence over the status. Is that correct?

If not (meaning we want to have parity), one thing we could do is to add a pill to the right of the site URL and maintain the status in its column.

@danielbachhuber
Copy link
Contributor Author

The site status doesn't matter if it's a redirect because the redirect takes effect immediately, so it has precedence over the status. Is that correct?

Correct. Good thing we're refactoring the Site Switcher soon!

Copy link
Contributor

@zaguiini zaguiini left a comment

Choose a reason for hiding this comment

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

Great work 👍

Copy link
Member

@sejas sejas left a comment

Choose a reason for hiding this comment

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

Code looks good.
Tested and working as described.

I've found that the redirect shows a wrong empty state:

Screenshot 2022-08-19 at 20 18 47

However, we can fix this in another PR.

if ( status === 'coming-soon' ) {
return (

@danielbachhuber
Copy link
Contributor Author

I've found that the redirect shows a wrong empty state:

@sejas Great catch! Updated with 5d81f98

image

Copy link
Contributor

@zaguiini zaguiini left a comment

Choose a reason for hiding this comment

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

Great that we have the "no redirect sites" state now 👍

@danielbachhuber danielbachhuber merged commit 5e795bb into trunk Aug 19, 2022
@danielbachhuber danielbachhuber deleted the improve/sites-management-page-redirect-sites branch August 19, 2022 19:51
@github-actions 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 Aug 19, 2022
@a8ci18n
Copy link

a8ci18n commented Aug 19, 2022

This Pull Request is now available for translation here: https://translate.wordpress.com/deliverables/7483033

Thank you @danielbachhuber for including a screenshot in the description! This is really helpful for our translators.

@p-jackson
Copy link
Member

Thanks for the quick fix!

I took a look at the ellipsis menu actions for redirects, and although they work, they're perhaps not that relevant for a site redirect. Something for i2 I think.

Status isn't necessarily the greatest grouping to put Redirect under. I think of Status as short for Launch status, and Redirect isn't anything to do with that. But on the other hand Status is a made-up concept just for this dashboard, so we can sort of use it for whatever we want.

I thought about maybe hiding the Redirect from the dropdown unless the user actually has a redirect site. But maybe it's good advertising.

CC @SaxonF just making sure he's aware of the changes.

@SaxonF
Copy link
Contributor

SaxonF commented Aug 22, 2022

This is a good first pass, nice work @danielbachhuber !

@danielbachhuber
Copy link
Contributor Author

Thanks for the quick fix!

@p-jackson You're welcome!

Status isn't necessarily the greatest grouping to put Redirect under. I think of Status as short for Launch status, and Redirect isn't anything to do with that. But on the other hand Status is a made-up concept just for this dashboard, so we can sort of use it for whatever we want.

Yeah, it's not a perfect abstraction. However, if Status represents the "state" of a site, then I think Redirect exists at the same level as Public, Private, and Coming Soon. Or, put another way, a Redirect will always redirect a user's site, so it doesn't matter if it's public or private.

I thought about maybe hiding the Redirect from the dropdown unless the user actually has a redirect site. But maybe it's good advertising.

I'd lean towards hiding from the dropdown unless there's at least one, too. Our Sites Management Page will become rapidly complex unless we actively keep the UI clear.

@p-jackson
Copy link
Member

Or, put another way, a Redirect will always redirect a user's site, so it doesn't matter if it's public or private.

That's true. Putting Redirect in the column is better than something like N/A!

@a8ci18n
Copy link

a8ci18n commented Aug 30, 2022

Translation for this Pull Request has now been finished.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Reviewer Can Merge PR author indicates the reviewer is free to merge/deploy if they want to own the change. Sites Management Page i1 [Type] Task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sites Management Page: Visually distinguish redirect sites
7 participants