This repository has been archived by the owner on Jan 24, 2024. It is now read-only.
Pattern clients #68
Closed
Pattern clients #68
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
Added clients images for creating patterns
Clients pattern added
|
This is what I'm seeing (though we should not be bundling those logos here anyhow). 
Also, we should be using wide/full width instead of manual content widths — otherwise the user can't change them holistically. 
|
|
@richtabor The logos are not loading in your end? |
|
|
@kafleg What does this code mean? I don't understand why you added it here as a comment, can you please explain? |
|
@carolinan Sorry for the confusion. I just shared it here for @richtabor to see if I worked as per his comment or not. |
|
Why are the images in the comment remote? |
Would you please check my new commit? |
|
@richtabor What do you propose using instead of bundling the fake logos? |
carolinan
reviewed
Aug 29, 2023
Maybe some of the image icons, if we're going to bundle those? Or linking to like images on WP photos. We really need placeholders to support height/width/aspect ratio so we don't have to bundle images. WordPress/gutenberg#48081 |
|
Like the asterisk icon, maybe just in place of each logo. |
patterns/clients.php
Outdated
| /** | ||
| * Title: Clients | ||
| * Slug: twentytwentyfour/clients | ||
| * Categories: columns |
There was a problem hiding this comment.
Columns is not one of the existing categories for patterns. Let's use text instead.
|
|
||
| ?> | ||
| <!-- wp:group {"tagName":"main","style":{"spacing":{"padding":{"top":"var:preset|spacing|80","right":"var:preset|spacing|20","bottom":"var:preset|spacing|80","left":"var:preset|spacing|20"},"margin":{"top":"0","bottom":"0"}}},"backgroundColor":"white","layout":{"type":"constrained","contentSize":"1320px","wideSize":"100%"}} --> | ||
| <main class="wp-block-group has-white-background-color has-background" style="margin-top:0;margin-bottom:0;padding-top:var(--wp--preset--spacing--80);padding-right:var(--wp--preset--spacing--20);padding-bottom:var(--wp--preset--spacing--80);padding-left:var(--wp--preset--spacing--20)"><!-- wp:group {"layout":{"type":"constrained","contentSize":"100%"}} --> |
There was a problem hiding this comment.
I don't think we should use the main tag here, since this is not the main container of the content of the site. Also, this group block should be wide aligned to fit the design
MaggieCabrera
suggested changes
Sep 4, 2023
Comment on lines
+24
to
+25
| <!-- /wp:group --></div> | ||
| <!-- /wp:group --></div> |
There was a problem hiding this comment.
I think we are stacking the logos into too many containers (row > columns > group > stack). Let's try and simplify this by instead of using columns altogether, just using one row block. If you check the figma, the columns are not the same width anyway, and using the row block you can control vertical alignment too.
| <!-- wp:column {"verticalAlignment":"center","width":"20%","layout":{"type":"default"}} --> | ||
| <div class="wp-block-column is-vertically-aligned-center" style="flex-basis:20%"><!-- wp:group {"style":{"spacing":{"padding":{"top":"var:preset|spacing|40","right":"var:preset|spacing|40","bottom":"var:preset|spacing|40","left":"var:preset|spacing|40"}}},"layout":{"type":"constrained","justifyContent":"center","contentSize":"1320px","wideSize":"100%"}} --> | ||
| <div class="wp-block-group" style="padding-top:var(--wp--preset--spacing--40);padding-right:var(--wp--preset--spacing--40);padding-bottom:var(--wp--preset--spacing--40);padding-left:var(--wp--preset--spacing--40)"><!-- wp:group {"style":{"border":{"radius":"8px","width":"0px","style":"none"}},"layout":{"type":"flex","orientation":"vertical","justifyContent":"center"}} --> | ||
| <div class="wp-block-group" style="border-style:none;border-width:0px;border-radius:8px"><!-- wp:image {"id":1104,"sizeSlug":"full","linkDestination":"none","style":{"border":{"radius":"8px"}}} --> |
There was a problem hiding this comment.
we need to remove border radius from all of the images
There was a problem hiding this comment.
And the images have an opacity to them, I don't think we have a simple way to achieve that. Maybe using duotone to fake it?
There was a problem hiding this comment.
Worse case scenario, let's change the images themselves
Merged
|
@MaggieCabrera @kafleg I have created a new PR with a simplified markup for this pattern. Please check it here #198 |
|
Thank you @maneshtimilsina |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.