Domains: Fix DNS pages header design issues #58692

leonardost · 2021-11-30T22:53:00Z

Changes proposed in this Pull Request

This PR fixes design issues in the DNS pages header pointed out by the design review in pcYYhz-q9-p2#comment-358.

The following screenshots highlight what was updated:

Button is now not primary and reads "+ Add a record"
Ellipsis icon is vertically aligned with the other items in the breadcrumbs
Inactive redo icon is now visible in menu
The "+ Add a record" button becomes a borderless "+" button in mobile view and the ellipsis menu is removed

You can compare them with the previous designs:

Note: Breadcrumbs issues are being addressed in #58601 and #58519.

Testing instructions

Build this branch locally or open the live Calypso link
- If you're in the live Calypso link, please append ?flags=domains/dns-records-redesign to your URL to enable the appropriate feature flag
Go to "Upgrades > Domains"
Select one of your registered domains or domain connections
Go to "Change your name servers & DNS records"
Ensure the design updates described above are present

github-actions · 2021-11-30T22:53:19Z

Link to Calypso live: https://calypso.live?image=registry.a8c.com/calypso/app:build-21622
Link to Jetpack Cloud live: https://calypso.live?image=registry.a8c.com/calypso/app:build-21622&env=jetpack

matticbot · 2021-11-30T22:54:19Z

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

Sections (~59 bytes added 📈 [gzipped])

name     parsed_size           gzip_size
domains       +204 B  (+0.0%)      +59 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.

leonardost · 2021-11-30T22:56:47Z

client/my-sites/domains/domain-management/dns/style.scss

+	.popover__menu-item[disabled] {
+		svg.gridicon {
+			fill: var( --color-neutral-20 );
+		}
+	}


Having to define this feels like a regression 🤔 the icon was invisible because it was being colored color: var( --color-neutral-0 ). Or maybe we're defining something wrong and the icon ends up being colored wrongly?

poligilad-auto · 2021-12-01T15:32:55Z

Thanks @leonardost!
Most of this looks good to me.

This is kind of my mistake, but I think the "3 dots" button should still be there in mobile, Like this:

“3 dots” this should be within a 40x40px button

This point doesn't seem to be addressed and this should be true for all borderless buttons (in any page, including the "Domains" page). This helps create consistent spacing as well as providing a large enough hit area on mobile.

Currently, it's like this:

While I think we should go for a similar approach to how it is in P2 for example. Their button size is 36x36. In our case it would be 40x40.

leonardost · 2021-12-01T23:06:53Z

Thanks for the review @poligilad-auto!

I think the "3 dots" button should still be there in mobile

I had thought about asking you about that but I didn't, so I'm at fault too 😆

The width of the ellipsis button is now 40px:

And it's in the mobile view too:

I reduced the left margin of the button to 8px in the mobile view as it seemed that way in the design - is this correct?

poligilad-auto · 2021-12-03T14:20:14Z

The width of the ellipsis button is now 40px

Perfect.

I reduced the left margin of the button to 8px in the mobile view as it seemed that way in the design - is this correct?

Yes, exactly correct 👌

The only thing now is doing the same for the "Domains" page :)

Now we also have miss-alignment that needs to be fixed in the table, that's what this comment was about:
"“3 dots”: let’s add 8px margin-right to this to align with the “3 dots” in the header, similar to my comment for the domains list."

rafaelgallani

Nice job! LGTM. Tested that all the described changes were reflected.

There's only one single issue I found with the TXT record alignment - although I'm not sure if that's related to this PR:

I think it's easier to fix it in this PR, but if that's not the case, then feel free to ignore this comment 😅
I also left a single code suggestion, but that's only a nitpick - it should be good to go with or without it.

rafaelgallani · 2021-12-07T05:38:49Z

client/my-sites/domains/domain-management/components/breadcrumbs/style.scss

@@ -171,6 +171,10 @@
 .breadcrumbs__buttons-mobile {
 	display: flex;

+	> * + * {


Nice one!
There's a relatively new flex/grid display CSS property called gap that does the same thing - and you can also define row-gap and column-gap separately on it.

I saw a few references to it in our codebase, although I'm not so sure how that would be interpreted by our webpack loaders. It might be worth giving a try 😸

I think it was Del who wrote that selector, it's neat 😄

I was going to say that selector isn't inside a grid context, but I just learned that gap can be used in flexbox too, that's nice!

Yeah...I think you should be able to use gap here. Probably more readable than the "owl-face" selector I used. :-D

🦉 Owl face, that's a nice one * + * 😄

leonardost · 2021-12-07T12:44:57Z

Thanks for the review @rafaelgalani and nice catch! I'll take a look at that case 👀

matticbot · 2021-12-07T18:00:16Z

This PR modifies the release build for wpcom-block-editor

To test your changes on WordPress.com, run install-plugin.sh wpcom-block-editor update/dns-pages-header-design-fixes on your sandbox.

To deploy your changes after merging, see the documentation: PCYsg-l4k-p2

matticbot · 2021-12-07T18:00:57Z

This PR modifies the release build for editing-toolkit

To test your changes on WordPress.com, run install-plugin.sh editing-toolkit update/dns-pages-header-design-fixes on your sandbox.

To deploy your changes after merging, see the documentation: PCYsg-mMA-p2

matticbot · 2021-12-07T18:01:52Z

This PR modifies the release build for notifications

To test your changes on WordPress.com, run install-plugin.sh notifications update/dns-pages-header-design-fixes on your sandbox.

To deploy your changes after merging, see the documentation: PCYsg-elI-p2

leonardost · 2021-12-07T19:01:53Z

The only thing now is doing the same for the "Domains" page :)

Cool! I've aligned the 3 dots icons with the header's 3 dots, I'll do the same for the "Domains" page in a follow-up PR 👍

a8ci18n · 2021-12-07T20:16:40Z

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

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

* Fix DNS page header design issues * Fix width of ellipsis breadcrumb button * Reduce left-margin of breadcrumb buttons in mobile view * Align DNS item rows' 3 dots with DNS header's 3 dots * Fix positioning of ellipsis menu of DNS items in mobile view * Make DNS items ellipsis popover appear below the icon instead of above

a8ci18n · 2021-12-17T13:29:00Z

Translation for this Pull Request has now been finished.

leonardost requested a review from a team as a code owner November 30, 2021 22:53

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

leonardost commented Nov 30, 2021

View reviewed changes

leonardost added the [Feature Group] Emails & Domains Features related to email integrations and domain management. label Nov 30, 2021

leonardost requested a review from a team December 1, 2021 14:30

leonardost added 3 commits December 6, 2021 21:06

Fix DNS page header design issues

9ce677c

Fix width of ellipsis breadcrumb button

dcd1aab

Reduce left-margin of breadcrumb buttons in mobile view

f2ed3c0

leonardost force-pushed the update/dns-pages-header-design-fixes branch from 7fa198b to f2ed3c0 Compare December 7, 2021 00:07

Align DNS item rows' 3 dots with DNS header's 3 dots

7115fd9

rafaelgallani reviewed Dec 7, 2021

View reviewed changes

Fix positioning of ellipsis menu of DNS items in mobile view

b29f745

Make DNS items ellipsis popover appear below the icon instead of above

39a6173

leonardost merged commit 8aa46e6 into trunk Dec 7, 2021

leonardost deleted the update/dns-pages-header-design-fixes branch December 7, 2021 19:33

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 Dec 7, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Domains: Fix DNS pages header design issues #58692

Domains: Fix DNS pages header design issues #58692

leonardost commented Nov 30, 2021 •

edited

Loading

github-actions bot commented Nov 30, 2021 •

edited by matticbot

Loading

matticbot commented Nov 30, 2021 •

edited

Loading

leonardost Nov 30, 2021

poligilad-auto commented Dec 1, 2021

leonardost commented Dec 1, 2021

poligilad-auto commented Dec 3, 2021

rafaelgallani left a comment

rafaelgallani Dec 7, 2021

leonardost Dec 7, 2021

delputnam Dec 7, 2021

leonardost Dec 7, 2021

leonardost commented Dec 7, 2021

matticbot commented Dec 7, 2021

matticbot commented Dec 7, 2021

matticbot commented Dec 7, 2021

leonardost commented Dec 7, 2021

a8ci18n commented Dec 7, 2021

a8ci18n commented Dec 17, 2021

Domains: Fix DNS pages header design issues #58692

Domains: Fix DNS pages header design issues #58692

Conversation

leonardost commented Nov 30, 2021 • edited Loading

Changes proposed in this Pull Request

Testing instructions

github-actions bot commented Nov 30, 2021 • edited by matticbot Loading

matticbot commented Nov 30, 2021 • edited Loading

leonardost Nov 30, 2021

Choose a reason for hiding this comment

poligilad-auto commented Dec 1, 2021

leonardost commented Dec 1, 2021

poligilad-auto commented Dec 3, 2021

rafaelgallani left a comment

Choose a reason for hiding this comment

rafaelgallani Dec 7, 2021

Choose a reason for hiding this comment

leonardost Dec 7, 2021

Choose a reason for hiding this comment

delputnam Dec 7, 2021

Choose a reason for hiding this comment

leonardost Dec 7, 2021

Choose a reason for hiding this comment

leonardost commented Dec 7, 2021

matticbot commented Dec 7, 2021

matticbot commented Dec 7, 2021

matticbot commented Dec 7, 2021

leonardost commented Dec 7, 2021

a8ci18n commented Dec 7, 2021

a8ci18n commented Dec 17, 2021

leonardost commented Nov 30, 2021 •

edited

Loading

github-actions bot commented Nov 30, 2021 •

edited by matticbot

Loading

matticbot commented Nov 30, 2021 •

edited

Loading