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

InlineHelp: Add admin help sections data #43790

Merged
merged 24 commits into from Jul 7, 2020

Conversation

retrofox
Copy link
Contributor

@retrofox retrofox commented Jun 30, 2020

Changes proposed in this Pull Request

This PR adds the help admin sections data to the inline help component. It's part of the process to restore the help admin section for the InlineHelp and HelpSearch cards.
The way to access the data is through of a state selector, named getAdminHelpResults().

Testing instructions

  • Take a look at the admin items defined in the admin-sections.js file. Check typos, descriptions, synonyms, etc.
  • Run unit tests
> yarn test-client client/state/inline-help/test/selectors.js
 PASS  client/state/inline-help/test/reducer.js
 PASS  client/state/inline-help/test/selectors.js

Test Suites: 2 passed, 2 total
Tests:       32 passed, 32 total
Snapshots:   0 total
Time:        3.431 s, estimated 4 s
Ran all test suites matching /client\/state\/inline-help\/test/i.

Watch Usage: Press w to show more.
✨  Done in 2249.24s.
> yarn test-client client/blocks/inline-help/test/admin-sections.js
 PASS  client/blocks/inline-help/test/admin-setions.js

Test Suites: 1 passed, 1 total
Tests:       5 passed, 5 total
Snapshots:   0 total
Time:        3.957 s
Ran all test suites matching /client\/blocks\/inline-help\/test\/admin-setions.js/i.
✨  Done in 8.54s.

All tests should pass.

Fixes #43464

@retrofox retrofox added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jun 30, 2020
@retrofox retrofox requested a review from a team June 30, 2020 11:18
@retrofox retrofox self-assigned this Jun 30, 2020
@matticbot
Copy link
Contributor

icon: 'domains',
},
{
title: translate( 'Change my site address' ),
Copy link

Choose a reason for hiding this comment

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

ℹ️ String reuse speeds up translation and improves consistency. The following string might make a good alternative and has already been translated 20 times:
translate( 'Change your site's address' ) ES Score: 7
See 1 additional suggestions in the PR translation status page

icon: 'cog',
},
{
title: translate( "Change my site's theme" ),
Copy link

Choose a reason for hiding this comment

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

ℹ️ String reuse speeds up translation and improves consistency. The following string might make a good alternative and has already been translated 20 times:
translate( 'Change your site's address' ) ES Score: 7

icon: 'plans',
},
{
title: translate( 'Cancel my plan' ),
Copy link

Choose a reason for hiding this comment

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

ℹ️ String reuse speeds up translation and improves consistency. The following string might make a good alternative and has already been translated 29 times:
translate( 'Can I cancel my plan?' ) ES Score: 8

icon: 'plans',
},
{
title: translate( 'Upgrade my plan' ),
Copy link

Choose a reason for hiding this comment

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

ℹ️ String reuse speeds up translation and improves consistency. The following string might make a good alternative and has already been translated 27 times:
translate( 'Can I upgrade my plan later?' ) ES Score: 7

icon: 'plans',
},
{
title: translate( 'Renew my plan' ),
Copy link

Choose a reason for hiding this comment

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

ℹ️ String reuse speeds up translation and improves consistency. The following string might make a good alternative and has already been translated 24 times:
translate( 'Does my plan auto-renew?' ) ES Score: 7

icon: 'user',
},
{
title: translate( "Change my site's timezone" ),
Copy link

Choose a reason for hiding this comment

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

ℹ️ String reuse speeds up translation and improves consistency. The following string might make a good alternative and has already been translated 20 times:
translate( 'Change your site's address' ) ES Score: 6

icon: 'cog',
},
{
title: translate( 'Update my profile' ),
Copy link

Choose a reason for hiding this comment

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

ℹ️ String reuse speeds up translation and improves consistency. The following string might make a good alternative and has already been translated 23 times:
translate( 'Update my profile picture' ) ES Score: 8
See 1 additional suggestions in the PR translation status page

{
title: translate( 'Switch the interface language' ),
description: translate(
'Update the language of the interface you see across WordPress.com as a whole.'
Copy link

Choose a reason for hiding this comment

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

ℹ️ String reuse speeds up translation and improves consistency. The following string might make a good alternative and has already been translated 37 times:
translate( 'This is the language of the interface you see across WordPress.com as a whole.' ) ES Score: 7

icon: 'cog',
},
{
title: translate( 'Close my account permanently' ),
Copy link

Choose a reason for hiding this comment

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

ℹ️ String reuse speeds up translation and improves consistency. The following string might make a good alternative and has already been translated 33 times:
translate( 'Close your account permanently' ) ES Score: 7

@matticbot
Copy link
Contributor

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.

@retrofox retrofox force-pushed the update/inline-help-add-admin-sections branch from 9694693 to 1735a16 Compare June 30, 2020 13:08
@getdave
Copy link
Contributor

getdave commented Jul 1, 2020

Suggest the translation suggestions get fixed in another PR.

Copy link
Contributor

@getdave getdave left a comment

Choose a reason for hiding this comment

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

Thanks for getting this prepared and for the new tests. Really great to see! 👍

I've run the tests and they all pass. However, I feel they are providing us with a false confidence about the function of getAdminSectionsResults because they rely on testing the implementation details rather than the public API of the function.

If we can rework the tests to

  • focus on the getAdminSectionsResults method's public API.
  • move tests for filterListBySearchTerm into a separate test file to isolate them.

...then we'll have tests which provide more confidence in our software and are more resilient to change.

Thanks again for all your work on this.

client/state/inline-help/test/selectors.js Outdated Show resolved Hide resolved
client/state/inline-help/test/selectors.js Outdated Show resolved Hide resolved
client/state/inline-help/test/selectors.js Outdated Show resolved Hide resolved
client/state/inline-help/test/selectors.js Outdated Show resolved Hide resolved
client/blocks/inline-help/admin-sections.js Show resolved Hide resolved
@retrofox
Copy link
Contributor Author

retrofox commented Jul 1, 2020

Thanks for getting this prepared and for the new tests. Really great to see! 👍

I've run the tests and they all pass. However, I feel they are providing us with a false confidence about the function of getAdminSectionsResults because they rely on testing the implementation details rather than the public API of the function.

Let me change the location of some files as well as change tests. Moving to in progress.

@retrofox
Copy link
Contributor Author

retrofox commented Jul 6, 2020

obenland moved this from 👀 Ready for Review to 💻 In progress in Ajax Iteration Board 4 days ago

Wondering why did you move it back to the review state. Not a biggie at all, just curious.

@retrofox retrofox force-pushed the update/inline-help-add-admin-sections branch from 715ac13 to c04f3b4 Compare July 6, 2020 11:16
@retrofox
Copy link
Contributor Author

retrofox commented Jul 6, 2020

@getdave I've updated the PR and it's ready for a new review round.

@obenland
Copy link
Member

obenland commented Jul 6, 2020

Wondering why did you move it back to the review state. Not a biggie at all, just curious.

Based on your comment earlier where you said

Moving to in progress.

@retrofox
Copy link
Contributor Author

retrofox commented Jul 6, 2020

Wondering why did you move it back to the review state. Not a biggie at all, just curious.

Based on your comment earlier where you said

Moving to in progress.

yes, well... I moved it back, did and pushed some changes to finally moved it forward again. Thanks for answering. 👍

@retrofox retrofox added [Feature] My Home The main dashboard for managing a WordPress.com site. and removed [Status] Needs Author Reply labels Jul 6, 2020
Copy link
Member

@obenland obenland left a comment

Choose a reason for hiding this comment

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

This PR is looking pretty straight forward.
I noticed an opportunity to send users to the applicable open sections in the Customizer and added those suggestions.

client/blocks/inline-help/admin-sections.js Outdated Show resolved Hide resolved
client/blocks/inline-help/admin-sections.js Outdated Show resolved Hide resolved
client/blocks/inline-help/admin-sections.js Outdated Show resolved Hide resolved
client/blocks/inline-help/admin-sections.js Show resolved Hide resolved
@retrofox retrofox force-pushed the update/inline-help-add-admin-sections branch from ac818eb to b964f15 Compare July 7, 2020 15:51
@retrofox retrofox merged commit 2ae8e5c into master Jul 7, 2020
@retrofox retrofox deleted the update/inline-help-add-admin-sections branch July 7, 2020 17:25
@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 Jul 7, 2020
@a8ci18n
Copy link

a8ci18n commented Jul 7, 2020

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

Hi @retrofox, could you please edit the description of this PR and add a screenshot for our translators? Ideally it'd include all of the following strings:

  • Manage my domain settings
  • Change my site address
  • Add a site redirect
  • Redirect your site to another domain.
  • Change my password
  • Change my site's theme
  • Customize my site's theme
  • Change my homepage
  • Edit my menu
  • Set a site logo
  • Find a plan to suit my site
  • Cancel my plan
  • Upgrade my plan
  • Cancel G Suite
  • Renew my plan
  • Renew my domain
  • View my site activity
  • View my site's latest stats
  • Upload an image, video, audio or document
  • Import content from another site
  • Earn money from my site
  • Learn how to market my site
  • Manage my site's users
  • Invite new users and edit existing ones.
  • Invite new users to my site
  • Change my site's timezone
  • Switch your site from private to public.
  • Delete a site or a site's content
  • Remove all posts, pages, and media, or delete a site completely.
  • Set a site icon
  • Change my site's footer text
  • Export my site's content and media library
  • Export posts, pages and more from your site.
  • Manage sharing and social media connections
  • Add sharing buttons to my site
  • Install, manage, and search for site Plugins
  • Approve or delete comments
  • Manage how users can comment on my site
  • Manage post categories
  • Edit my site title, tagline, or logo
  • Set up a podcast
  • Change my site's privacy settings
  • Manage SEO and traffic settings
  • Update my profile
  • Update your name, profile image, and about text.
  • Update my username or email address
  • Change the dashboard color scheme
  • Switch the interface language
  • Update the language of the interface you see across WordPress.com as a whole.
  • Close my account permanently
  • Change my account privacy settings
  • View my purchase and billing history
  • Download the WordPress.com app for my device
  • View my drafted posts
  • Manage my blog posts
  • View my drafted pages
  • Manage my pages
  • I cannot find my site on Google
  • Verify my site with Google
  • View contact form messages
  • Portfolio projects (for those who have them active)

Thank you in advance!

@jancavan
Copy link
Contributor

jancavan commented Jul 7, 2020

Nice work @retrofox! This is working as expected.

For the design, can we please just remove the chevron like the mockup here. I don't think they add any value and can be confusing in pages that display a "Back" button.

Can we also make the icon color different from the link color:

Normal: color-gray-30

Screen Shot 2020-07-07 at 11 57 23 AM

Hover: color-gray-50

Screen Shot 2020-07-07 at 11 57 47 AM

Active state with white icon looks good 👍

@jancavan
Copy link
Contributor

jancavan commented Jul 7, 2020

Additional feedback:

  • For multi-line results, icon needs to be aligned top. Is this PR not reflecting the correct icons yet?

Screen Shot 2020-07-07 at 1 26 12 PM

  • We should return the same admin results for any word containing domain. Currently, domains does not return anything as well as buy a domain

  • As mentioned here, it would really be nice if we could anchor the user to the appropriate card. For example, searching for footer text should take me to the "Footer credit" card. This can be handled separately if need be.

  • If both support articles and admin return no results, we can display as is, with a single message. But I just noticed the current layout is a little confusing. It's telling me there are no results, but links below it are being displayed looking like there actually are results. I think we should add a heading like below to make that clear:

Screen Shot 2020-07-07 at 2 33 18 PM

  • If there no admin results, we can display just support articles. I don't think we need to highlight that there are no admin results like this. It doesn't add anything, and might only emphasize our currently limited results.

Screen Shot 2020-07-07 at 2 35 52 PM

  • Are there any instances where support will return nothing, but admin results will? I can't think of an example, but let me know!

Thanks @retrofox! This is looking really good.

@retrofox
Copy link
Contributor Author

retrofox commented Jul 7, 2020

thanks @jancavan for your review. Almost all issues have been addressed, less

it would really be nice if we could anchor the user to the appropriate card.

Agree. I've done a few a little research a few days ago and I'm afraid it deserves a dedicated PR. Guessing it isn't a big deal but it's kind of out of the scope of this PR.

If there no admin results, we can display just support articles.

I couldn't reproduce this visual issue. 🤷

@jancavan
Copy link
Contributor

jancavan commented Jul 8, 2020

@retrofox I realized the other branch: #43862 has already been merged, so going to just add the color values here:

Normal:
color: var( --color-neutral-light );

Hover:
color: var( --color-neutral );

Active/is-selected:
color: var( --color-text-inverted );

@retrofox
Copy link
Contributor Author

retrofox commented Jul 8, 2020

Thanks, @jancavan. Going to create a PR for this.

@jancavan
Copy link
Contributor

jancavan commented Jul 8, 2020

@retrofox No need to create one :) Already about to push out a PR.

@jancavan
Copy link
Contributor

jancavan commented Jul 8, 2020

Agree. I've done a few a little research a few days ago and I'm afraid it deserves a dedicated PR. Guessing it isn't a big deal but it's kind of out of the scope of this PR.

@retrofox Not sure how much would this would entail, but do you know if this is out of scope just for this PR, or out of scope for the project itself?

@retrofox
Copy link
Contributor Author

retrofox commented Jul 8, 2020

if this is out of scope just for this PR, or out of scope for the project itself?

My first assumption is for this PR. :-)

@getdave
Copy link
Contributor

getdave commented Jul 9, 2020

Also noting that we need to update the e2e tests to cover the new admin results.

@retrofox
Copy link
Contributor Author

retrofox commented Jul 9, 2020

Also noting that we need to update the e2e tests to cover the new admin results.

I've created an issue for this. #44025

@a8ci18n
Copy link

a8ci18n commented Jul 16, 2020

Translation for this Pull Request has now been finished.

},
{
title: translate( 'Manage my blog posts' ),
link: '/posts/${ siteSlug }',
Copy link
Member

Choose a reason for hiding this comment

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

Seems like this should have been a string literal. Fix is in #45539.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] My Home The main dashboard for managing a WordPress.com site.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(3P) Settings Search: Review admin sections & commit
8 participants