-
Notifications
You must be signed in to change notification settings - Fork 801
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
Add LiveChat icon to WPAdmin on Atomic only. #18885
Conversation
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 If you are an automattician, once your PR is ready for review add the "[Status] Needs Team review" label and ask someone from your team review the code. jetpack plugin:
|
Caution: This PR has changes that must be merged to WordPress.com |
@frontdevde I'd really appreciate your help here with:
|
Pushed a couple of updates, the issues were related to
Hope that helps! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'll need to fix those linting issues before we can merge this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work so far, @getdave. I only took a first quick look without testing it, but I think this is going in the right direction.
My only concern is that this WP Admin FAB will conflict with the Calypso FAB on screens that we embed through an iframe (e.g. block editor / Gutenframe). It may be good to check the non-presence of the frame-nonce
GET param before injecting the WP Admin FAB, since that param indicates the screen is going to be embedded on a iframe.
It'd be also good to solve all the linting warnings/errors.
projects/plugins/jetpack/modules/masterbar/inline-help/class-inline-help.php
Outdated
Show resolved
Hide resolved
projects/plugins/jetpack/modules/masterbar/inline-help/class-inline-help.php
Outdated
Show resolved
Hide resolved
projects/plugins/jetpack/modules/masterbar/inline-help/class-inline-help.php
Outdated
Show resolved
Hide resolved
@mmtr I fixed the doubling up of the icons in Gutenframe. @frontdevde Just need to tidy up the CSS to move things to Sass and avoid CSS vars. |
Kudos to @frontdevde for this. |
At the moment this is blocked by me not being sure of the best way to get the diff to be in sync with these changes. |
You should be able to commandeer D57354-code and update the diff after manually copy/paste the |
Waiting on review of accompanying diff which covers Simple sites. D57354-code |
projects/plugins/jetpack/modules/masterbar/inline-help/inline-help-template.php
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me. 👍
projects/plugins/jetpack/modules/masterbar/inline-help/class-inline-help.php
Show resolved
Hide resolved
Co-authored-by: Miguel Torres <[email protected]> Co-authored-by: Michael P. Pfeiffer <[email protected]>
Cherry-picked to |
Adds the "FAB" LiveChat icon to WPAdmin on WordPress.com envs only. This is a non-interactive element as it is not yet possible to "embed" the actual FAB widget from Calypso into WPAdmin. This acts as a temporary workaround to allow the user access to "help" via a direct link to
wordpress.com/help
.To do this we must:
In addition we must use the existing color schemes unification process to define custom color overides for the icon depending on which color scheme is being used in WPAdmin. This involves adding new vars to each color scheme file and then applying these to the FAB styling.
Fixes Automattic/wp-calypso#50271
Fixes Automattic/wp-calypso#50367
Please note
Changes proposed in this Pull Request:
Jetpack product discussion
See paYJgx-1nM-p2
Does this pull request change what data or activity we track or use?
No
Testing instructions:
The following steps should be taken on both Simple & Atomic (the visual icon check should be tested across a variety of screen sizes):
wordpress.com/help
.wordpress.com
and open your site in Calypso.Color Schemes
Atomic sites
Simple sites
This doesn't work yet because we need to update JP Fusion to sync the
inline-help
directory within masterbar to Dotcom Simple codebase.Once this PR is provisionally accepted for Atomic sites then we'll merge this diff (D57403-code) to update JP sync and then rebase all the things.
Proposed changelog entry for your changes: