-
Notifications
You must be signed in to change notification settings - Fork 31
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
Feature/21 An SVG Gutenberg Block #80
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.
@faisal-alvi thanks for working on this.
I have used this image to test:
Few things I would like to point out:
- When you first select the SVG, the rendered image dimensions is very small, even when Image size is set to Full Size.
- When you first select the SVG, the default values set are: Full Size, 200x200, 100%, but when you switch to Medium and back to Full Size, the dimension changes to 575x188.
- The SVG width doesn't stretch to 100% width of the editor when setting dimension to 100%.
- Without an SVG selected, the controls are practically dead. We can hide settings if the SVG is not selected.
Please watch the video for more details, I have compared it with the core/image
block.
PR-80.mp4
Note
PR #79 uses 10up-toolkit whereas this one uses wp-scripts. We need to decide which one to go ahead with and make changes to the other PR accordingly.
I just reviewed this code and it seems to be outputting an On top of that, only admins will be able to save a post with this block since it's saving an |
@jeffpaul @faisal-alvi I have to agree with @cr0ybot here. The reason the original pro version of the plugin used a dynamic block, was to get around the issue with the We're already restricting who can upload in #76 and unless we also want to give those roles the I suggest we stick to a dynamically rendered block. Original implementation for reference: https://github.com/10up/wpsvg/blob/develop/gutenberg/class-wpsvg-gutenberg.php#L103-L194 |
@cr0ybot @darylldoyle thank you for the feedback, it all makes sense. I will go ahead and tweak the code to make the block dynamically rendered at the front end. I assume that we have to keep the |
That sounds right to me. Without |
@Sidsector9 thanks for the feedback.
done
done
I don't know if that was an issue, but I've checked after the recent changes and we can set the SVG image to cover 100% of the available width.
done |
@darylldoyle @Sidsector9 any further code review feedback here or is this good to move along to merge/release? |
@faisal-alvi note some merge conflicts to get cleaned up here, please and thanks! |
Conflicts resolved: - package.json - tests/cypress/plugins/index.js
LGTM! Nice work 🙂 |
Some quick feedback for this new block:
In case anyone is looking for disabling the new block: // Disable Safe SVG block.
add_action(
'plugins_loaded',
static function(): void {
global $safe_svg;
if ( ! isset( $safe_svg ) ) {
return;
}
remove_action( 'init', [ $safe_svg, 'setup_blocks' ] );
}
); |
Description of the Change
image link
image link
image link
image link
Closes #21
How to test the Change
Changelog Entry
Credits
Props @faisal-alvi @jeffpaul @Sidsector9
Checklist: