-
Notifications
You must be signed in to change notification settings - Fork 14
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
[CIVIC-1204] Added SVG support for Logos. #1093
Conversation
* @return bool | ||
* TRUE if the file path has SVG extension, FALSE otherwise. | ||
*/ | ||
function is_svg_extension($file_path) { |
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.
Please rename to civictheme_file_is_svg()
* File path to check. | ||
* | ||
* @return bool | ||
* TRUE if the file path has SVG extension, FALSE otherwise. |
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.
TRUE if the file is SVG, FALSE otherwise.
We do not want to disclose the implementation in comments.
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.
but we may no need this function at all since the SVG file can be passed to the <img>
tag as a src
attribute value.
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.
@joshua-salsadigital
Please see inline comments.
Also, please update Storybook story for Logo to include using of SVG logos - replace the following PNG logos with SVG logos
logo_primary_dark_desktop.png
logo_primary_dark_mobile.png
logo_primary_light_desktop.png
logo_primary_light_mobile.png
The SVG logos are attached to the JIRA ticket.
Thank you
@@ -195,7 +197,7 @@ public function form(&$form, FormStateInterface &$form_state) { | |||
'@public' => rtrim($this->toFriendlyFilePath($this->getDefaultFileScheme()), '/'), | |||
]), | |||
'#upload_validators' => [ | |||
'file_validate_is_image' => [], | |||
'file_validate_extensions' => [implode(' ', $allowed_extensions)], |
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.
please revert - we do not support SVG in background images
@@ -26,6 +26,8 @@ public function weight() { | |||
* @SuppressWarnings(PHPMD.ExcessiveMethodLength) | |||
*/ | |||
public function form(&$form, FormStateInterface &$form_state) { | |||
$allowed_extensions = $this->imageFactory->getSupportedExtensions(); |
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.
please move these closer to line 97 where it is actually used.
alt: img.alt, | ||
modifier_class: 'ct-logo__image ct-logo__image--' ~ breakpoint ~ ' ' ~ (breakpoint == 'mobile' ? (type == 'inline' and logo_type == 'secondary' ? 'hide-xxs' : 'hide-l') : 'hide-xxs show-l'), | ||
} only %} | ||
{% if img.svg_logo %} |
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.
Please revert this and pass SVG file directly to <img>
tag
@@ -43,9 +44,25 @@ function civictheme_preprocess_block__system_branding_block(&$variables) { | |||
foreach (['desktop', 'mobile'] as $breakpoint) { | |||
$logo_image = civictheme_get_theme_config_manager()->load("components.logo.{$type}.{$theme}.{$breakpoint}.path", ''); | |||
if (!empty($logo_image)) { | |||
if (is_svg_extension($logo_image)) { |
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.
revert all this - see above
@@ -217,6 +217,31 @@ Feature: Components settings are available in the theme settings | |||
And I check the box "Confirm settings reset" | |||
And I press "reset_to_defaults" | |||
Then I should see the text "Theme configuration was reset to defaults." | |||
|
|||
@api |
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.
👍
https://salsadigital.atlassian.net/browse/CIVIC-1204
Checklist before requesting a review
[CIVIC-123] Verb in past tense with dot at the end.
Changed
section about WHY something was done if this was not a normal implementationChanged
Screenshots