Feature/dswp 62 secondary hero image with title #44
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ASpiteri-BCGov
requested review from
kshapka-bcgov,
robrien-bcgov and
ShawnTurple
ShawnTurple
reviewed
Jan 6, 2025
| <div class="wp-block-group" style="padding-right:var(--wp--preset--spacing--80);padding-left:var(--wp--preset--spacing--80)"><!-- wp:columns {"verticalAlignment":"center","style":{"spacing":{"padding":{"right":"0","left":"0","top":"var:preset|spacing|40","bottom":"var:preset|spacing|40"},"margin":{"top":"0","bottom":"0"}}}} --> | ||
| <div class="wp-block-columns are-vertically-aligned-center" style="margin-top:0;margin-bottom:0;padding-top:var(--wp--preset--spacing--40);padding-right:0;padding-bottom:var(--wp--preset--spacing--40);padding-left:0"><!-- wp:column {"verticalAlignment":"center","width":"18%","style":{"spacing":{"padding":{"right":"0","left":"0","top":"var:preset|spacing|60","bottom":"var:preset|spacing|60"},"blockGap":"0"}},"layout":{"type":"default"}} --> | ||
| <div class="wp-block-column is-vertically-aligned-center" style="padding-top:var(--wp--preset--spacing--60);padding-right:0;padding-bottom:var(--wp--preset--spacing--60);padding-left:0;flex-basis:18%"><!-- wp:image {"align":"left","id":15402,"width":"180px","aspectRatio":"1","scale":"cover","sizeSlug":"full","linkDestination":"none","className":"is-style-rounded"} --> | ||
| <figure class="wp-block-image alignleft size-full is-resized is-style-rounded"><img src="<?php echo esc_url( get_template_directory_uri() . '/assets/images/square-512.png' ); ?>" alt="" class="wp-image-15402" style="aspect-ratio:1;object-fit:cover;width:180px" /></figure> | ||
| <figure class="wp-block-image alignleft size-full is-resized is-style-rounded"><img src="https://localhost/ticorp/wp-content/themes/design-system-wordpress-theme/assets/images/square-512.png" alt="" class="wp-image-15402" style="aspect-ratio:1;object-fit:cover;width:180px" /></figure> |
There was a problem hiding this comment.
so i'm guessing this will only work when you are developing locally, did something not work the original way.
ShawnTurple
reviewed
Jan 6, 2025
| "slug": "menu-color-background", | ||
| "color": "var(--dswp-surface-color-menus-default)", | ||
| "name": "Menu background color" | ||
| "slug": "background-light-gray", |
There was a problem hiding this comment.
as we discussed, using colour names like gray, white, are going to be problematic when re-using these colours or knowing where the colours are being used. the theme json colours probably should have something like widget-background, primary-border, or something, this colour could be used for. if we are keeping css vars they might be ok to use them in that context
ShawnTurple
reviewed
Jan 6, 2025
| <div class="wp-block-group" style="padding-right:var(--wp--preset--spacing--80);padding-left:var(--wp--preset--spacing--80)"><!-- wp:columns {"verticalAlignment":"center","style":{"spacing":{"padding":{"right":"0","left":"0","top":"var:preset|spacing|40","bottom":"var:preset|spacing|40"},"margin":{"top":"0","bottom":"0"}}}} --> | ||
| <div class="wp-block-columns are-vertically-aligned-center" style="margin-top:0;margin-bottom:0;padding-top:var(--wp--preset--spacing--40);padding-right:0;padding-bottom:var(--wp--preset--spacing--40);padding-left:0"><!-- wp:column {"verticalAlignment":"center","width":"18%","style":{"spacing":{"padding":{"right":"0","left":"0","top":"var:preset|spacing|60","bottom":"var:preset|spacing|60"},"blockGap":"0"}},"layout":{"type":"default"}} --> | ||
| <div class="wp-block-column is-vertically-aligned-center" style="padding-top:var(--wp--preset--spacing--60);padding-right:0;padding-bottom:var(--wp--preset--spacing--60);padding-left:0;flex-basis:18%"><!-- wp:image {"align":"left","id":15402,"width":"180px","aspectRatio":"1","scale":"cover","sizeSlug":"full","linkDestination":"none","className":"is-style-rounded"} --> | ||
| <figure class="wp-block-image alignleft size-full is-resized is-style-rounded"><img src="<?php echo esc_url( get_template_directory_uri() . '/assets/images/square-512.png' ); ?>" alt="" class="wp-image-15402" style="aspect-ratio:1;object-fit:cover;width:180px" /></figure> | ||
| <figure class="wp-block-image alignleft size-full is-resized is-style-rounded"><img src="https://localhost/ticorp/wp-content/themes/design-system-wordpress-theme/assets/images/square-512.png" alt="" class="wp-image-15402" style="aspect-ratio:1;object-fit:cover;width:180px" /></figure> |
There was a problem hiding this comment.
this is using localhost, i'm guessing if we can fix this to be more generic, we need to. this probably happens when you extract a saved version, and put it back into the file
ShawnTurple
reviewed
Jan 6, 2025
| <!-- wp:group {"align":"full","style":{"color":{"background":"#f5f5f5"},"spacing":{"padding":{"top":"var:preset|spacing|30","bottom":"var:preset|spacing|30"}}},"layout":{"type":"default"}} --> | ||
| <div class="wp-block-group alignfull has-background" style="background-color:#f5f5f5;padding-top:var(--wp--preset--spacing--30);padding-bottom:var(--wp--preset--spacing--30)"><!-- wp:group {"style":{"spacing":{"padding":{"right":"var:preset|spacing|80","left":"var:preset|spacing|80"}}},"layout":{"type":"constrained"}} --> | ||
| <!-- wp:group {"align":"full","style":{"spacing":{"padding":{"top":"var:preset|spacing|30","bottom":"var:preset|spacing|30"}}},"backgroundColor":"medium-gray","layout":{"type":"default"}} --> | ||
| <div class="wp-block-group alignfull has-medium-gray-background-color has-background" style="padding-top:var(--wp--preset--spacing--30);padding-bottom:var(--wp--preset--spacing--30)"><!-- wp:group {"style":{"spacing":{"padding":{"right":"var:preset|spacing|80","left":"var:preset|spacing|80"}}},"layout":{"type":"constrained"}} --> |
There was a problem hiding this comment.
so from my understanding, this is using one of the color pallett, and if it gets update to red, this background color would change
ShawnTurple
reviewed
Jan 6, 2025
| @@ -97,7 +97,7 @@ | |||
| <div class="wp-block-group" style="padding-right:var(--wp--preset--spacing--80);padding-left:var(--wp--preset--spacing--80)"><!-- wp:columns {"verticalAlignment":"center","style":{"spacing":{"padding":{"right":"0","left":"0","top":"var:preset|spacing|40","bottom":"var:preset|spacing|40"},"margin":{"top":"0","bottom":"0"}}}} --> | |||
| <div class="wp-block-columns are-vertically-aligned-center" style="margin-top:0;margin-bottom:0;padding-top:var(--wp--preset--spacing--40);padding-right:0;padding-bottom:var(--wp--preset--spacing--40);padding-left:0"><!-- wp:column {"verticalAlignment":"center","width":"18%","style":{"spacing":{"padding":{"right":"0","left":"0","top":"var:preset|spacing|60","bottom":"var:preset|spacing|60"},"blockGap":"0"}},"layout":{"type":"default"}} --> | |||
| <div class="wp-block-column is-vertically-aligned-center" style="padding-top:var(--wp--preset--spacing--60);padding-right:0;padding-bottom:var(--wp--preset--spacing--60);padding-left:0;flex-basis:18%"><!-- wp:image {"align":"left","id":15402,"width":"180px","aspectRatio":"1","scale":"cover","sizeSlug":"full","linkDestination":"none","className":"is-style-rounded"} --> | |||
| <figure class="wp-block-image alignleft size-full is-resized is-style-rounded"><img src="<?php echo esc_url( get_template_directory_uri() . '/assets/images/square-512.png' ); ?>" alt="" class="wp-image-15402" style="aspect-ratio:1;object-fit:cover;width:180px" /></figure> | |||
| <figure class="wp-block-image alignleft size-full is-resized is-style-rounded"><img src="https://localhost/ticorp/wp-content/themes/design-system-wordpress-theme/assets/images/square-512.png" alt="" class="wp-image-15402" style="aspect-ratio:1;object-fit:cover;width:180px" /></figure> | |||
There was a problem hiding this comment.
using resource from localhost
ShawnTurple
reviewed
Jan 6, 2025
| @@ -13,7 +13,7 @@ | |||
| <div class="wp-block-group" style="padding-right:var(--wp--preset--spacing--80);padding-left:var(--wp--preset--spacing--80)"><!-- wp:columns {"verticalAlignment":"center","style":{"spacing":{"padding":{"right":"0","left":"0","top":"var:preset|spacing|40","bottom":"var:preset|spacing|40"},"margin":{"top":"0","bottom":"0"}}}} --> | |||
| <div class="wp-block-columns are-vertically-aligned-center" style="margin-top:0;margin-bottom:0;padding-top:var(--wp--preset--spacing--40);padding-right:0;padding-bottom:var(--wp--preset--spacing--40);padding-left:0"><!-- wp:column {"verticalAlignment":"center","width":"18%","style":{"spacing":{"padding":{"right":"0","left":"0","top":"var:preset|spacing|60","bottom":"var:preset|spacing|60"},"blockGap":"0"}},"layout":{"type":"default"}} --> | |||
| <div class="wp-block-column is-vertically-aligned-center" style="padding-top:var(--wp--preset--spacing--60);padding-right:0;padding-bottom:var(--wp--preset--spacing--60);padding-left:0;flex-basis:18%"><!-- wp:image {"align":"left","id":15402,"width":"180px","aspectRatio":"1","scale":"cover","sizeSlug":"full","linkDestination":"none","className":"is-style-rounded"} --> | |||
| <figure class="wp-block-image alignleft size-full is-resized is-style-rounded"><img src="<?php echo esc_url( get_template_directory_uri() . '/assets/images/square-512.png' ); ?>" alt="" class="wp-image-15402" style="aspect-ratio:1;object-fit:cover;width:180px" /></figure> | |||
| <figure class="wp-block-image alignleft size-full is-resized is-style-rounded"><img src="https://localhost/ticorp/wp-content/themes/design-system-wordpress-theme/assets/images/square-512.png" alt="" class="wp-image-15402" style="aspect-ratio:1;object-fit:cover;width:180px" /></figure> | |||
There was a problem hiding this comment.
using resource from localhost
ShawnTurple
reviewed
Jan 6, 2025
| <?php | ||
| /** | ||
| * Title: DSWP Secondary Hero Image With Title | ||
| * Slug: design-system-wordpress-theme/dswp-secondary-hero-image-with-title |
There was a problem hiding this comment.
sorry, if this was already discuss, but just making sure there is a naming convention we are using, if so, seems like there might be some duplication or unnecessary naming for example if we have a name space of design-system-wordpress-theme do we need the name of the pattern starting off with dswp-
ShawnTurple
reviewed
Jan 6, 2025
| <!-- /wp:column --></div> | ||
| <!-- /wp:columns --></div> | ||
| <!-- wp:paragraph {"style":{"spacing":{"padding":{"top":"0","bottom":"0"}}}} --> | ||
| <p style="padding-top:0;padding-bottom:0"><a href="https://localhost/ticorp/test2/" data-type="page" data-id="15259">Neque porro quisquan</a></p> |
There was a problem hiding this comment.
using localhost with ticorp sitename
ShawnTurple
reviewed
Jan 6, 2025
| <div class="wp-block-group" style="margin-top:0;margin-bottom:0"><!-- wp:group {"style":{"spacing":{"padding":{"top":"var:preset|spacing|30","bottom":"var:preset|spacing|50","left":"var:preset|spacing|60","right":"var:preset|spacing|60"},"margin":{"top":"0","bottom":"0"}},"dimensions":{"minHeight":"278px"},"border":{"radius":"4px","left":{"color":"var:preset|color|banner-background-dark","width":"6px"},"top":{},"right":{},"bottom":{}}},"backgroundColor":"background-white","layout":{"type":"default"}} --> | ||
| <div class="wp-block-group has-background-white-background-color has-background" style="border-radius:4px;border-left-color:var(--wp--preset--color--banner-background-dark);border-left-width:6px;min-height:278px;margin-top:0;margin-bottom:0;padding-top:var(--wp--preset--spacing--30);padding-right:var(--wp--preset--spacing--60);padding-bottom:var(--wp--preset--spacing--50);padding-left:var(--wp--preset--spacing--60)"><!-- wp:group {"style":{"spacing":{"padding":{"right":"0","left":"0","top":"0","bottom":"0"},"margin":{"top":"var:preset|spacing|50","bottom":"var:preset|spacing|30"}}},"layout":{"type":"constrained"}} --> | ||
| <div class="wp-block-group" style="margin-top:var(--wp--preset--spacing--50);margin-bottom:var(--wp--preset--spacing--30);padding-top:0;padding-right:0;padding-bottom:0;padding-left:0"><!-- wp:heading {"textAlign":"left","style":{"elements":{"link":{"color":{"text":"var:preset|color|banner-background-dark"}}},"spacing":{"padding":{"top":"0","bottom":"0"},"margin":{"top":"0","bottom":"0"}}},"textColor":"banner-background-dark","fontSize":"large"} --> | ||
| <h2 class="wp-block-heading has-text-align-left has-banner-background-dark-color has-text-color has-link-color has-large-font-size" style="margin-top:0;margin-bottom:0;padding-top:0;padding-bottom:0">Transportation Investment Corporation</h2> |
There was a problem hiding this comment.
Not sure if we want to leave a specific site name in the design system theme.
ShawnTurple
requested changes
Jan 7, 2025
There was a problem hiding this comment.
Need To:
- update localhost references out of the assets
- remove any reference to a specific site name (ie TIcorp), only ipsum lorum
Observation:
- some of the patterns are using the default theme color pallet (adds classnames ... has-something-color), this is only a problem, if we allow these pallet colors to be updated via the interface or overridden in the child theme.
- using pallet colors naming convention with a color name, ie background-light-gray, or medium-gray
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.