Add pattern events - three columns with images and title (static)#185
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @[email protected]. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
@beafialho Displaying the date above the headings is problematic because for example visitors who use screen readers and navigate by the headings will not access the information about the date; or the date will be associate with the heading before it. Where do you want the date moved to? |
carolinan
added
the
[Status] Needs Design Feedback
| */ | ||
|
|
||
| ?> | ||
| <!-- wp:group {"metadata":{"categories":["featured"],"patternName":"twentytwentyfive/events-three-columns-images-and-titles","name":"Events three columns with event images and titles"},"align":"wide","style":{"spacing":{"padding":{"top":"var:preset|spacing|50","bottom":"var:preset|spacing|50"},"blockGap":"0"}},"layout":{"type":"constrained"}} --> |
There was a problem hiding this comment.
Please remove the metadata.
There was a problem hiding this comment.
The outer group block needs to be full width, otherwise there is no left and right spacing when a background color is added.
Please set the top and bottom margin to 0, so that there is no extra spacing between full width sections:
| <h3 class="wp-block-heading" style="padding-top:var(--wp--preset--spacing--20)">Tell your story</h3> | ||
| <!-- /wp:heading --> | ||
|
|
||
| <!-- wp:paragraph {"style":{"spacing":{"padding":{"top":"var:preset|spacing|40"}}},"fontSize":"large"} --> |
There was a problem hiding this comment.
Large is the default size for the paragraph, so it is not needed here. Remember to also remove the class name on the <p> tag below.
There was a problem hiding this comment.
Please remove it from all 3 paragraphs.
| <!-- /wp:heading --> | ||
|
|
||
| <!-- wp:paragraph {"style":{"spacing":{"padding":{"top":"var:preset|spacing|40"}}},"fontSize":"large"} --> | ||
| <p class="has-large-font-size" style="padding-top:var(--wp--preset--spacing--40)"><a href="#a">Event details</a></p> |
There was a problem hiding this comment.
Why is there an a inside the href? Did you mean for this to be #?
|
Hi @carolinan I made the correction to the pattern: removed metadata, make outer group fullwidth, set top and bottom margins to zero, use default size for event details paragraph and yes I meant # instead of #a (sorry). You also mention the class of the paragraph, I guess is the events details paragraph, but there is no class rightnow. So I guess is ok. I will wait for @beafialho's feedback to place the headings in the right place. Thanks a lot! And sorry for the errors, it's my first PR to a WP repo. By the way there are conflicts in the readme file due to the fact all of us add image licenses to it. Is it ok to make the PR like this? I guess the person in charge of merging it will solve them. |
|
Thank you for working on this @dballari! Let's move the date below the titles. Just to note because I see you added top padding to the date. Since it's now moving below the title, that top padding should be removed. For right space between title and date, I suggest grouping them with a stack block and set block spacing to the first preset.
|
juanfra
left a comment
juanfra
left a comment
There was a problem hiding this comment.
Thank you @dballari! I left some comments, adding to what Carolina and Bea mentioned.
By the way there are conflicts in the readme file due to the fact all of us add image licenses to it. Is it ok to make the PR like this? I guess the person in charge of merging it will solve them.
This can happen often. As you mentioned, in this particular case it is the readme.txt. The idea is that the contributor is the one that should be taking care of the conflicts, as it is the one knowing what the goal of the PR is, what's that it is solving and how it should work. Reviewers can probably catch problems, but ideally it is the contributor that's working on the PR the one that should be fixing the merge conflicts.
I'll take care of this first time, so that I maybe add some clarity on how it should be approached. Thanks again.
Co-authored-by: Juan Aldasoro <[email protected]>
Co-authored-by: Juan Aldasoro <[email protected]>
Co-authored-by: Juan Aldasoro <[email protected]>
4 participants
Description
Fixes #84 #84
Adding the ‘events three columns with images and titles’ pattern.
At first I implemented this pattern as a dynamic one using a Grid Custom Query Loop block, which is in another branch of my forked repo. The problem is the custom query has to be configured targeting the desired pieces of content (events). I guess for a default theme is better as an static pattern.
Screenshots
2024-09-03.09-26-36-addpattern-events-three-columns.mp4
Testing Instructions