Add new notice about latest release #2798
Conversation
There was a problem hiding this comment.
Looks great overall! First time taking a look at stats so forgive my ignorance on topics already discussed.
- Why do we have stats disabled by default? Generally, most people will simply dismiss notices without reading them and we shouldn't hide exciting new functionality behind settings I think. Maybe the actions in the notice should be 'Dismiss' and a redirect to settings for 'Disable'?
- I think the notice itself is a bit too big as it takes almost half of my screen. Is it possible to reduce its height a bit?
- I don't think that we should display the notice in new installations as it seems that it is directed to existing users.
There is also some minor Psalm errors
| wp_die( esc_html__( 'You don’t have permission to do this.', 'wp-job-manager' ) ); | ||
| } | ||
|
|
||
| do_action( 'job_manager_action_' . $action ); |
There was a problem hiding this comment.
Personally I would avoid dynamic hook name because they are hard to find and document. You can pass the action as an argument
There was a problem hiding this comment.
This is a pretty common pattern in WordPress and WPJM, like wp_ajax_*, admin_action_*. A bit harder to search but cleaner and faster to use.
There was a problem hiding this comment.
Yeah whenever I encountered it in the existing cases it always needed extra time to figure out. It usually goes like this:
- You use the hotkey to go to the hook definition. This doesn't work because the IDE can't figure out dynamic hooks.
- Then I search for the whole name
- Then I search for parts of the hook. It is also very common during this step for the results to be too many as the parts of the hook name are very commonly used for other things like variables or function names.
I don't see how dynamic hooks are cleaner as instead of doing concatenation we will pass an extra argument. The only downside is an extra check on the called method which I don't think is important enough.
|
Set up the plugin install/update hook to enable stats for new installs and skip this notice. Note that to test updating, the version in Also tweaked the size a bit, but the idea for this to be a little piece of news, I think we can take some real estate for it. It's only shown on the Job Manager screen — which eventually would be a 'Home' dashboard for news and things requiring action. I didn't want to complicate things and used the notice style, but we can tone it down a little and make this a more like News box:
|
There was a problem hiding this comment.
Looks better now!
Note that to test updating, the version in JOB_MANAGER_VERSION has to be bumped.
Probably I'm missing something here as I didn't need to do any updates for the notice to show.
This comment made me think though, how are we going to handle the release notices in versions after 2.3.0. We maybe don't want to show the notice to users upgrading to 2.4.0 directly. Maybe we shouldn't mention the version at all in the notice? Or maybe add an extra step to the release steps to stop showing the Release_Notice class. Maybe it isn't big of a deal though and we could just show it to everyone updating to a a post-2.3.0 version.
There is also the issue with the dynamic hook but I don't consider it a blocker
Right now the idea is to update the |
|
Makes sense! Could we add a comment in the release steps as something to check? |
Co-authored-by: gikaragia <[email protected]>
|
Updated the release checklist here: P6r3EZ-1Q-p2 |
Fixes #2796
Changes Proposed in this Pull Request
Testing Instructions
wp option delete wp_job_manager_dismissed_noticesto un-dismiss all noticesScreenshot / Video