-
Notifications
You must be signed in to change notification settings - Fork 34
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
Move playlist notice and add check for other notices #371
Conversation
@@ -21,11 +21,23 @@ public function __construct() { | |||
* Adds all templates for Backbone application | |||
*/ | |||
public function add_templates() { | |||
?> | |||
global $pagenow; ?> |
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.
global $pagenow; ?> | |
global $pagenow; | |
?> |
<# if( data.mediaType === 'playlists' || data.mediaType === 'playlistexperience' ) { #> | ||
<div class="notice notice-warning"> | ||
<p> | ||
<?php esc_html_e( 'Please note that you can create new playlists only from Brightcove.', 'brightcove' ); ?> | ||
</p> | ||
</div> | ||
<# } #> |
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.
I'm not sure I understand this one, @MARQAS. If you are testing for playlistexperience
before, do we want to display another very similar message?
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.
@felipeelia, this was already present, I just moved them. I believe it is there for a reason, There are two different sections, one is to add a Playlist, and the other one is to add a Playlist experience. So it tells the user in the playlist experience section, that you cannot add a new playlist and also you cannot add the new experiences from here as well.
Do you want me to combine them both and say: "Please note that you can create new Playlists or new Experiences only from Brightcove"
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.
Between lines 1292 and 1319 we have 3 ifs, all of them testing data.mediaType
. If mediaType is playlistexperience
the code will go through 2 ifs (1305 and 1313), and the user will see these two messages at the same time:
Please note that you can create new Experiences only from Brightcove.
Please note that you can create new playlists only from Brightcove.
Do you think that makes sense from the user's perspective? Were you able to reproduce that specific scenario?
Description of the Change
This PR moves the playlist notice in the admin section outside the controls bar and adds an additional check for notices to show them in respective places
Closes #346
How to test the Change
Changelog Entry
Credits
Props @MARQAS , @felipeelia
Checklist: