-
-
Notifications
You must be signed in to change notification settings - Fork 0
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
Shortcode Features - Add Shortcode Column and Copy To Clipboard feature #28
Conversation
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.
Overall code looks good. I've requested a few changes.
src/js/utils/index.js
Outdated
|
||
// Copy the value to the clipboard. | ||
navigator.clipboard.writeText( shortcodeValue ).then( () => { | ||
// Logs success message in console. |
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.
There needs to be a check to make sure this object is available in browsers such as Firefox.
See feature detection in this article: https://web.dev/articles/async-clipboard#feature_detection
src/js/utils/index.js
Outdated
const spanElement = button.querySelector( 'span' ); | ||
if ( spanElement ) { | ||
spanElement.classList.remove( 'dashicons', 'dashicons-clipboard' ); | ||
} |
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.
Can we change this to a checkbox when copied and use wp.a11y.speak
to communicate the button state?
https://make.wordpress.org/accessibility/handbook/markup/wp-a11y-speak/
// Generate the shortcode for the button with dashicon. | ||
$button_shortcode = sprintf( | ||
'<button type="button" class="pw-copy-clipboard" title="Copy to Clipboard">%s</button>', | ||
'<span class="dashicons dashicons-clipboard"></span>' |
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.
Need an aria-label
here.
|
||
// Generate the shortcode for the readonly input. | ||
$shortcode = sprintf( | ||
'<input type="text" value="%s" readonly>', |
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.
Can you make sure this input is disabled?
Resolves #26
This pull request consists of the following features below: