Enhancement/11433-pue-pointer #11673
Conversation
|
Storybook is ready:
|
|
Size Change: +397 B (+0.02%) Total Size: 2.19 MB ℹ️ View Unchanged
|
|
Build files for a6a3b26 have been deleted. |
zutigrm
left a comment
zutigrm
left a comment
There was a problem hiding this comment.
Thanks @benbowler mostly looks good, I left you some comments
assets/sass/components/wp-dashboard/_googlesitekit-wp-dashboard.scss
Outdated
Show resolved
Hide resolved
zutigrm
left a comment
zutigrm
left a comment
There was a problem hiding this comment.
LGTM
I removed the commneted out code in scss file
…e-kit-wp into enhancement/11433-pue-pointer.
There was a problem hiding this comment.
Thanks @benbowler, I've flagged a few things to address here, some of which should have been addressed in IBR so apologies for the late points.
Let me know if you have any questions or would like to discuss anything more for efficiency.
Also, a few checks are failing. Are these related? If not, they may be fixed in develop – this branch is currently 150+ commits behind so please remember to merge that in often.
assets/sass/components/wp-dashboard/googlesitekit-pointers.scss
Outdated
Show resolved
Hide resolved
assets/sass/components/wp-dashboard/googlesitekit-pointers.scss
Outdated
Show resolved
Hide resolved
tests/phpunit/integration/Core/Email_Reporting/Email_Reporting_PointerTest.php
Show resolved
Hide resolved
tests/phpunit/integration/Core/Email_Reporting/Email_Reporting_PointerTest.php
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| private function register_permissions_for_user( $user_id ) { | ||
| wp_set_current_user( $user_id ); |
There was a problem hiding this comment.
This line shouldn't be necessary since the user ID referenced below should always be from User_Options where it's already passed. It's unexpected that this method may also change the current user.
There was a problem hiding this comment.
I had to retain this with the updates to the permissions to be set correctly. Let me know if I'm missing a cleaner way to do this:
There was a problem hiding this comment.
It is needed to work, I was just saying it's better to do in the test than in this utility which is specific to setting permissions. I've gone ahead updated it – there was some additional simplifications that I was able to do as well.
tests/phpunit/integration/Core/Email_Reporting/Email_Reporting_PointerTest.php
Outdated
Show resolved
Hide resolved
|
Thanks @aaemnnosttv, I've addressed most points above.
I changed my mind, see below. |
|
Addressing this in a follow-up would affect the existing View Only Pointer, so I have updated the button implementation so that the |
|
Test failure is unrelated:
|
aaemnnosttv
left a comment
aaemnnosttv
left a comment
There was a problem hiding this comment.
Thanks @benbowler – there are still a few things to address here. One thing which really needed some reworking (in general) was the inline pointer JS which I've found a better way to do which also handles much of the escaping and avoids injecting dynamic JS from PHP so I'll push this up at the same time.
| import { CORE_UI } from '@/js/googlesitekit/datastore/ui/constants'; | ||
| import { USER_SETTINGS_SELECTION_PANEL_OPENED_KEY } from './email-reporting/constants'; | ||
|
|
||
| export default function CoreDashboardEffects() { |
There was a problem hiding this comment.
One important difference here with the Module version is that modules can register two different effect components – one for the main and entity dashboards respectively. Core doesn't need to register its effects so its components are simpler, but it would make sense to follow the same pattern where this top level component renders the dashboard-specific variant based on the context.
If this is getting out of scope we could address it in a follow up too, I just wanted to highlight this detail for maintaining consistency.
There was a problem hiding this comment.
I've continued with a single component for now across main and entity dashboards and created #11712 to split this out in future.
assets/sass/components/wp-dashboard/googlesitekit-pointers.scss
Outdated
Show resolved
Hide resolved
| * limitations under the License. | ||
| */ | ||
|
|
||
| .googlesitekit-pointer-cta--dismiss { |
There was a problem hiding this comment.
This doesn't quite look like the design. This is what I see
There was a problem hiding this comment.
In the designs the X icon takes the color from the Pointer header, however, to maintain compatibility with the WP theme the pointer header will take the primary color of the theme. Therefore I went with black for this icon to work across all possible customised theme colors.
There was a problem hiding this comment.
Let's iterate on this in a follow up as we should be able to match it even with custom admin themes.
aaemnnosttv
left a comment
aaemnnosttv
left a comment
There was a problem hiding this comment.
Thanks again @benbowler, I think this is pretty close now. I've left a few more comments and also tried to explain some of the changes I made – apologies for making as many as I did!
| 'data-slug' => $pointer->get_slug(), | ||
| 'data-class' => implode( ' ', $class ), | ||
| 'data-target-id' => $pointer->get_target_id(), | ||
| 'data-title' => wp_kses( $pointer->get_title(), $kses_title ), | ||
| 'data-content' => wp_kses( $content, $kses_content ), | ||
| 'data-position' => wp_json_encode( $pointer->get_position() ), |
There was a problem hiding this comment.
These will all be escaped as attributes automatically so no escaping is needed here, but the values should still be sanitized/cleaned with wp_kses where relevant. Most importantly, no content is injected directly into JS anymore.
| } | ||
|
|
||
| public function test_get_buttons() { | ||
| $buttons = 'function(event, container) { return jQuery("<button>OK</button>"); }'; |
There was a problem hiding this comment.
This should be updated since JS can't be passed through from this class any more.
| private Context $context; | ||
|
|
||
| private User_Options $user_options; | ||
|
|
||
| private Dismissed_Items $dismissed_items; | ||
|
|
||
| private User_Email_Reporting_Settings $email_reporting_settings; |
There was a problem hiding this comment.
We don't require the full doc blocks for test classes so I've made this more succinct with typed properties.
| use Google\Site_Kit\Core\Modules\Module_Sharing_Settings; | ||
| use Google\Site_Kit\Core\Storage\Options; | ||
| use Google\Site_Kit\Core\Storage\User_Options; | ||
| use Google\Site_Kit\Core\User\Email_Reporting_Settings as User_Email_Reporting_Settings; |
There was a problem hiding this comment.
Looking at this now, why are we aliasing the name?
| * @group Util | ||
| * @group Email_Reporting | ||
| */ | ||
| class Email_Reporting_PointerTest extends TestCase { |
There was a problem hiding this comment.
I made a fair number of simplifications here. For example, a lot of the permissions related code was unnecessary.
Co-authored-by: Evan Mattson <[email protected]>
…e-kit-wp into enhancement/11433-pue-pointer.
|
Thanks @aaemnnosttv, I have addressed the outstanding comments I could see and fixed issues with dismissal that were introduced. I have added a number of comments inline above. |
aaemnnosttv
left a comment
aaemnnosttv
left a comment
There was a problem hiding this comment.
Thanks @benbowler , just a very few minor updates to make and a merge conflict which I can resolve and then this is good to go.
| } | ||
|
|
||
| public function test_get_buttons() { | ||
| $buttons = 'function(event, container) { return jQuery("<button>OK</button>"); }'; |
There was a problem hiding this comment.
The fact that this test passes with this input somewhat sets the expectation that this method is intended to return JS and would work although that's no longer the case.
| /** | ||
| * WordPress dependencies | ||
| */ | ||
| import { useMount } from 'react-use'; |
There was a problem hiding this comment.
This is an external dependency, not WP.
| 'title' => sprintf( | ||
| '%s %s', | ||
| __( 'You now have access to Site Kit', 'google-site-kit' ), | ||
| '<button type=\'button\' class=\'googlesitekit-pointer-cta--dismiss dashicons dashicons-no\' data-action=\'dismiss\'>' . |
There was a problem hiding this comment.
Let's not escape the inner quotes unnecessarily.
| 'class' => 'googlesitekit-view-only-pointer', | ||
| 'buttons' => | ||
| sprintf( | ||
| '<a class=\'googlesitekit-pointer-cta button-primary\' href=\'admin.php?page=googlesitekit-dashboard\' data-action=\'dismiss\'>%s</a>', |
There was a problem hiding this comment.
Same here, but also the href should be absolute.
|
The failing JS test is an unrelated instability in |
aaemnnosttv
left a comment
aaemnnosttv
left a comment
There was a problem hiding this comment.
LGTM, thanks @benbowler!
3 participants
Summary
Addresses issue:
Relevant technical choices
PR Author Checklist
Do not alter or remove anything below. The following sections will be managed by moderators only.
Code Reviewer Checklist
Merge Reviewer Checklist