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.

Sign up for GitHub

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

Jump to bottom

Focused Launch: Fix domain picker item flex spacing #49883

Merged
merged 2 commits into from Feb 18, 2021
Merged

Focused Launch: Fix domain picker item flex spacing #49883

merged 2 commits into from Feb 18, 2021

Conversation

alshakero
Copy link
Member

@alshakero alshakero commented Feb 9, 2021 •
edited

Changes proposed in this Pull Request

  • This PR tries to avoid the too-early breaking of domain picker items.
  • This issue seems deceptively easy to fix. I tried several ideas but none of them met all the condititons (IE support and works in both domain picker modes). I ended up with a very simple (probably simplistic) solution of limited the width of the price column when the domain is free.

Why is it hard?

In each domain item, there two competing columns. The domain name column, and the price column. We prefer the domain picker to stay in one line, but not too rigid to make the phrase "Included in paid plans" narrow enough to render one word per line. We need the domain picker to stay in one line as long as it doesn't squeeze the price too much.

The current solution is to split the columns by 50:50 and remove the competition between the two columns entirely. This tells the domain name to stay in one line as long as it's shorter than 50% of the item. Easy and clean enough.

In free domains though, 50% is way too much for the price column, because they don't have the phrase "Included in paid plans". This PR adds a special case for free domains to have a narrower price column. As for the paid domains, I think the 50:50 makes the most sense.

Testing instructions

  1. Create or reuse a site that is created with /start flow.
  2. Sandbox this site.
  3. In apps/editing-toolkit run yarn dev --sync.
  4. Go to https://[yoursite].wordpress.com/wp-admin/post-new.php
  5. Click launch.
  6. In the domain picker step, verify that Focused launch: free sub-domain cuts to 2nd line too early #49817 is fixed.

Regression testing

  1. Go to calypso.live/new?branch=fix/domain-picker-flex
  2. Reach the domain step.
  3. Verify that Focused launch: free sub-domain cuts to 2nd line too early #49817 is fixed.
  4. Using BrowserStack, you can test this in IE11 too.

Fixes #49817

@matticbot
Copy link
Contributor

Test live: https://calypso.live/?branch=fix/domain-picker-flex

@alshakero alshakero changed the title Fix domain picker item flex spacing Focused Launch: Fix domain picker item flex spacing Feb 9, 2021
@alshakero alshakero added the Focused Launch Issues and PRs related to Focused Launch label Feb 9, 2021
@matticbot
Copy link
Contributor

matticbot commented Feb 9, 2021 •
edited

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

@ciampo ciampo requested a review from a team February 10, 2021 08:59
@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 Feb 10, 2021
@ciampo ciampo added this to Needs review in Focused Launch (inactive) via automation Feb 10, 2021
@ciampo ciampo added the [Feature Group] Emails & Domains Features related to email integrations and domain management. label Feb 10, 2021
StefanNieuwenhuis
StefanNieuwenhuis reviewed Feb 10, 2021
View reviewed changes
Copy link
Contributor

@StefanNieuwenhuis StefanNieuwenhuis left a comment

Choose a reason for hiding this comment

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

Awesome job! The domain suggestions won't be cut off prematurely, but can you please address my two other comments?

Screenshot 2021-02-10 at 15 17 22

packages/domain-picker/src/domain-picker/style.scss Outdated
@@ -232,8 +232,7 @@ $accent-blue: #117ac9;
}

.domain-picker__suggestion-item-name {
flex-grow: 1;
flex-basis: 2px;
width: max-content;
Copy link
Contributor

Choose a reason for hiding this comment

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

Internet Explorer doesn't provide support for max-content (see MDN Docs). Should we consider this an issue?

packages/domain-picker/src/domain-picker/style.scss Outdated
@@ -232,8 +232,7 @@ $accent-blue: #117ac9;
}

.domain-picker__suggestion-item-name {
flex-grow: 1;
flex-basis: 2px;
width: max-content;
Copy link
Contributor

Choose a reason for hiding this comment

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

"For text content this means that the content will not wrap at all even if it causes overflows" - .

The max-content property will prevent content from wrapping and will eventually cause overflows (see MDN Docs). In our case, this happens when the URL is (extremely) long (see screenshot). Do you think we need to address and fix this?

Screenshot 2021-02-10 at 15 27 11

@razvanpapadopol
Copy link

razvanpapadopol commented Feb 11, 2021 •
edited

As @StefanNieuwenhuis mentioned, cross-browser and cross-device testing is welcome for any CSS change and not only.

Another point is about a regression on /new where the domain items are no longer aligned to left.
Desktop:
Screenshot 2021-02-11 at 10 10 13

Mobile:
Screenshot_20210211-093007_Chrome

Since the change is in a shared component, let's make sure to include other use cases in the Testing instructions as well.

@razvanpapadopol razvanpapadopol moved this from Needs review to In progress in Focused Launch (inactive) Feb 11, 2021
@alshakero alshakero added [Status] In Progress and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Feb 11, 2021
@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 Feb 17, 2021
@ciampo ciampo moved this from In progress to Needs review in Focused Launch (inactive) Feb 17, 2021
StefanNieuwenhuis
StefanNieuwenhuis approved these changes Feb 18, 2021
View reviewed changes
Copy link
Contributor

@StefanNieuwenhuis StefanNieuwenhuis left a comment

Choose a reason for hiding this comment

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

I took it for another spin and the changes LGTM sine I don't see any premature breaking of domain picker items anymore! 🚢

Focused Launch (inactive) automation moved this from Needs review to Ready to merge Feb 18, 2021
@alshakero alshakero merged commit a74d6ea into trunk Feb 18, 2021
Focused Launch (inactive) automation moved this from Ready to merge to Done Feb 18, 2021
@alshakero alshakero deleted the fix/domain-picker-flex branch February 18, 2021 12:18
@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 Feb 18, 2021
@ciampo ciampo mentioned this pull request Feb 18, 2021
Merged
20 tasks
@razvanpapadopol razvanpapadopol mentioned this pull request Mar 1, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@razvanpapadopol razvanpapadopol razvanpapadopol left review comments

@StefanNieuwenhuis StefanNieuwenhuis StefanNieuwenhuis approved these changes

Assignees

@alshakero alshakero

Labels
[Feature Group] Emails & Domains Features related to email integrations and domain management. Focused Launch Issues and PRs related to Focused Launch
Projects
Focused Launch (inactive)
  
🎉 Done (yay us!)
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Focused launch: free sub-domain cuts to 2nd line too early
5 participants
@alshakero @matticbot @razvanpapadopol @StefanNieuwenhuis @ciampo