Skip to content
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.

Sign up for GitHub

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

Quadrat: Add alignments for image cover and video #3816

Closed
wants to merge 2 commits into from
Closed

Quadrat: Add alignments for image cover and video #3816

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5973aef
Quadrat: Add aligment rules
scruffian May 10, 2021
159c22b
Quadrat: Add aligment rules
scruffian May 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
232 changes: 146 additions & 86 deletions blank-canvas-blocks/assets/ponyfill.css
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions blank-canvas-blocks/experimental-theme.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,10 @@
},
"margin": {
"baseline": "10px",
"bodyHorizontal": "0px",
"bodyVertical": "0px",
"bodyHorizontalDesktop": "0px",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm concerned that by adding more custom variables within the theme that are media query specific, we are going down a similar path to alignments that won't be compatible with FSE.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think that these variables should be set by the Site Editor?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I rather think they make sense as variables in a theme for re-use, but (IMO) doesn't HAVE to be surfaced to the user (and is unlikely to be a standard way). So they make sense in a child theme, just not BCB. I'm not sure that I'm keen for most of this to be in BCB; it all feels very theme-specific and something unlikely to be handled by gutenberg (thus not something to be handled by BCB).

Copy link
Contributor

@pbking pbking May 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we have a standard method of describing padding perhaps it would be better to define this just like any other element. And to take a note from the button hover state refactor, perhaps could also express the mobile 'state' in the same way. i.e.

		"custom": {
			"alignment": {
				"alignedMaxWidth": "50%"
			},
			"body": {
				"spacing": {
					"padding": {
						"top": "0px",
						"bottom": "0px",
						"left": "0px",
						"right": "0px"
					}
				},
				"mobile": {
					"spacing": {
						"padding": {
							"top": "0px",
							"bottom": "0px",
							"left": "0px",
							"right": "0px"
						}
					}
				}
			},

It's more verbose but follows the convention of Gutenberg.

"bodyVerticalDesktop": "0px",
"horizontal": "30px",
"vertical": "30px"
},
Expand Down
23 changes: 19 additions & 4 deletions blank-canvas-blocks/sass/base/_alignment.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
}

@include break-mobile {
// limit size of any element that is aligned left/right
// limit size of any element that is aligned left/right
.wp-block[data-align="left"], // This is for the editor
.wp-block[data-align="right"], // This is for the editor
.wp-site-blocks .alignleft,
Expand All @@ -15,9 +15,9 @@
}

// When content is aligned left/right (particularly inside of a container) it is floated left/right
// and needs something to ensure that the content follows the block rather than nestling up beside the floated element.
// and needs something to ensure that the content follows the block rather than nestling up beside the floated element.
// The issue should be resolved upstream: https://github.com/WordPress/gutenberg/issues/10299
.wp-block-group:not(.site-header) {
.wp-block-group:not(.site-header) {
overflow: auto;
}

Expand All @@ -26,4 +26,19 @@
// class which would do this for us. I'm not sure why but this centers things appropriately.
.aligncenter {
text-align: center;
}
}

// These blocks go right to the edge of the viewport
.wp-block-image,
.wp-block-cover,
.wp-block-video {
&.alignfull {
margin: 0 calc(-1 * var(--wp--custom--margin--body-horizontal));
width: calc(100% + var(--wp--custom--margin--body-horizontal)*2);

@include break-mobile {
margin: 0 calc(-1 * var(--wp--custom--margin--body-horizontal-desktop));
width: calc(100% + var(--wp--custom--margin--body-horizontal-desktop)*2);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redefining variables (as noted below in _body.scss) would eliminate the need for additional breakpoints like this. There would just be one calculation, the value of the variable changing depending on the viewport width.

}
}
9 changes: 9 additions & 0 deletions blank-canvas-blocks/sass/elements/_body.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
@import '../base/breakpoints';

body {
padding: var(--wp--custom--margin--body-vertical) var(--wp--custom--margin--body-horizontal);

@include break-mobile {
padding: var(--wp--custom--margin--body-vertical-desktop) var(--wp--custom--margin--body-horizontal-desktop);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leveraging and then redefining the variables here would allow for other places that use the body padding variables to do so only once (the value of the variable would change based on the breakpoint)

body {
	padding-top: var(--wp--custom--body--padding--top);
	padding-bottom: var(--wp--custom--body--padding--bottom);
	padding-left: var(--wp--custom--body--padding--left);
	padding-right: var(--wp--custom--body--padding--right);

	@include break-mobile-only {
		--wp--custom--body--padding--top: var(--wp--custom--body--mobile--padding--top);		
		--wp--custom--body--padding--bottom: var(--wp--custom--body--mobile--padding--bottom);		
		--wp--custom--body--padding--left: var(--wp--custom--body--mobile--padding--left);		
		--wp--custom--body--padding--right: var(--wp--custom--body--mobile--padding--right);		
	}
}

}
3 changes: 2 additions & 1 deletion blank-canvas-blocks/sass/elements/_style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,5 +3,6 @@
* - Styles for basic HTML elemants
*/

@import "body";
@import "links";
@import "forms";
@import "forms";
10 changes: 5 additions & 5 deletions blank-canvas-blocks/sass/ponyfill.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,6 @@
// - Reset the browser
@import "base/style";

// Elements
// - Styles for basic HTML elemants
@import "elements/style";

// Blocks
// - These styles replace key Gutenberg Block styles for fonts, colors, and
// spacing with CSS-variables overrides
Expand All @@ -26,4 +22,8 @@
@import "blocks/table";
@import "blocks/video";
@import "blocks/columns";
@import "post/meta";
@import "post/meta";

// Elements
// - Styles for basic HTML elemants
@import "elements/style";
4 changes: 2 additions & 2 deletions quadrat/block-template-parts/header.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
<!-- wp:group {"className":"site-header","style":{"spacing":{"padding":{"right":"35px","left":"35px"}}}} -->
<div class="wp-block-group site-header" style="padding-right:35px;padding-left:35px">
<!-- wp:group {"className":"site-header"} -->
<div class="wp-block-group site-header">
<!-- wp:site-title /-->
<!-- wp:navigation {"orientation":"horizontal","textColor":"foreground-light","itemsJustification":"right","fontSize":"small"} -->
<!-- wp:navigation-link {"label":"Home","url":"#"} /-->
Expand Down
2 changes: 2 additions & 0 deletions quadrat/child-experimental-theme.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,8 @@
}
},
"margin": {
"bodyHorizontal": "20px",
"bodyHorizontalDesktop": "35px",
"horizontal": "20px",
"vertical": "30px"
},
Expand Down
10 changes: 7 additions & 3 deletions quadrat/experimental-theme.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,10 @@
},
"margin": {
"baseline": "10px",
"bodyHorizontal": "20px",
"bodyVertical": "0px",
"bodyHorizontalDesktop": "35px",
"bodyVerticalDesktop": "0px",
"horizontal": "20px",
"vertical": "30px"
},
Expand Down Expand Up @@ -193,8 +197,8 @@
},
"post-content": {
"padding": {
"left": "var(--wp--custom--margin--horizontal)",
"right": "var(--wp--custom--margin--horizontal)"
"left": "0px",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to look at the impact of this change

"right": "0px"
}
},
"pullquote": {
Expand Down Expand Up @@ -518,4 +522,4 @@
"fontWeight": "300"
}
}
}
}
Loading