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.

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

Proposal: marginFromParent property #2015

Closed
andrewleader opened this issue Oct 10, 2018 · 4 comments
Closed

Proposal: marginFromParent property #2015

andrewleader opened this issue Oct 10, 2018 · 4 comments

Comments

@andrewleader
Copy link
Contributor

andrewleader commented Oct 10, 2018

Solves requests

Summary

Allow controlling the margins that parents add to children, so that bleeding background images and colors can be achieved.

This takes a different perspective, one where the children can choose to remove the "padding" that the parent introduces.

This can be implemented by changing the rendering pattern so that children apply the "padding" - actually a margin in that case. So a parent tells its children "Here's the margin I would like", but the children can choose to ignore that.

This makes for a much simpler authoring story.

Schema

New property on all body elements.

Property Type Required Description
marginFromParent "auto" or Spacing or Thickness false Controls how much margin from the parent container will be added

Why the name marginFromParent rather than just margin? It's because this only affects the margin between the item and its parent (so left and right sides, and if it's the first item, the top, and if it's the last item, the bottom). It doesn't affect spacing between items (which is what the spacing property is for).

Thickness type

Property Type Required Description
left "auto" or Spacing false The desired thickness on the left
top "auto" or Spacing false The desired thickness on the top
right "auto" or Spacing false The desired thickness on the right
bottom "auto" or Spacing false The desired thickness on the bottom

Example

Imagine a developer has the following emphasis container, and they want that to bleed to the edges of the card.

image

{
    "type": "AdaptiveCard",
    "version": "1.0",
    "body": [
        {
            "type": "Container",
            "style": "emphasis",
            "items": [
                {
                    "type": "TextBlock",
                    "text": "This container has a gray background that extends to the edges of the card",
                    "wrap": true
                }
            ]
        },
        {
            "type": "Container",
            "items": [
                {
                    "type": "TextBlock",
                    "text": "This other container does not have a background.",
                    "wrap": true
                }
            ]
        }
    ]
}

They would simply add "marginFromParent": "none" to the container to remove the margins around the container...

{
    "type": "AdaptiveCard",
    "version": "1.0",
    "body": [
        {
            "type": "Container",
            "style": "emphasis",
            "marginFromParent": "none",
            "items": [
                {
                    "type": "TextBlock",
                    "text": "This container has a gray background that extends to the edges of the card",
                    "wrap": true
                }
            ]
        },
        {
            "type": "Container",
            "items": [
                {
                    "type": "TextBlock",
                    "text": "This other container does not have a background.",
                    "wrap": true
                }
            ]
        }
    ]
}

image

Creating better tags

This can be used to control the padding around items to create better tags

tagmargin

Divider headers

This makes it super easy to turn headers into full-bleed divider headers. It only takes the addition of one single property to each header, nothing else.

dividerheaders

Don't worry what elements are around

As an author, you don't have to worry about whether this is the last element in the card, or whether there are elements above or below you. It "just works".

Notice how the spacing between the last two elements doesn't change, and when we add a new last element, everything still looks correct.

lastelement

Host Config

No changes.

Down-level impact

Low. The margins aren't correctly displayed, but content is still functional.

Host burden

None. Renderer handles it all.

@dclaux
Copy link
Member

dclaux commented Oct 10, 2018

Looking at this on a technical standpoint

This proposal makes the wrong assumption that containers have a margin by default; they don't. They have or don't have padding depending on whether they have a background (either an image or via a container style.) The card itself always has padding. Padding is necessary in a container that has a background so that the text doesn't stick to the container's edges which would look terrible. But all elements (including containers) are subject to their parent container's (or the card itself) padding.

Example:
image

Payload:

{
	"$schema": "https://adaptivecards.io/schemas/adaptive-card.json",
	"type": "AdaptiveCard",
	"version": "1.0",
	"body": [
		{
            "type": "Container",
            "style": "emphasis",
            "items": [
                {
                    "type": "TextBlock",
                    "text": "This container has padding because it has a background. The padding ensures this text is not stuck at the top-left which would look bad.",
                    "wrap": true
                }
            ]
		}
	]
}

If the container didn't have padding, it would look like this:
image

But the container doesn't have any margin. The white space around it is the card's padding.

To have the container bleed to the edges of the card, we can't remove something that isn't there in the first place (e.g. margins.) We need to remove the padding on the card itself:
image

Payload - note the "padding": "none" property on the card:

{
	"$schema": "https://adaptivecards.io/schemas/adaptive-card.json",
	"type": "AdaptiveCard",
	"version": "1.0",
    "padding": "none",
    "body": [
		{
            "type": "Container",
            "style": "emphasis",
            "items": [
                {
                    "type": "TextBlock",
                    "text": "This container has padding because it has a background. The padding ensures this text is not stuck at the top-left which would look bad. The container extends to the edges of the card because the card itself doesn't have paddings.",
                    "wrap": true
                }
            ]
		}
	]
}

You can try the above payload and play with the padding property (on the card and any container) in Outlook's Card Playground tool.

Looking at this on a functional standpoint

Maybe the term "margin" isn't the right one. But maybe a "bleed" boolean property would make sense; when "bleed" is set to true on a Container, it would bleed into its parent container's padding. This is typically done by applying negative margins to the container that should bleed. But:

  • Although most platforms support negative margins, not all of them do
  • Implementing this model is somewhat complicated, depending how far we want to go. It is basically already implemented (although turned off by default) in the HTML renderer. Its complexity is what lead me to propose explicit paddings (padding property on Container and ColumnSet #1727) as a much simpler alternative.

@andrewleader
Copy link
Contributor Author

It all depends on your perspective. What we have today can be accomplished either with margins or padding, they're functionally equivalent.

Imagine for a second that for containers...

  • All containers have padding (even ones without different backgrounds)
  • Containers receive a margin if their background is different

For example, imagine that a plain container as seen below simply has no margin (flush to the border of the card), but has padding, as illustrated with the red box (yes I know the red box currently represents the card, but choose to see a different perspective)...

image

Now, when we place emphasis on that container, imagine that the container becomes inset with a margin (but it keeps the same padding it previously had)...

image

The developer could then choose to remove that inset/margin to achieve the borderless experience.

It's certainly a bit more complicated from the rendering side, but it removes the concerns that Matt and Bill had.

@dclaux
Copy link
Member

dclaux commented Oct 10, 2018

Let me make a bold statement: 100% of our renderers use paddings to accomplish the results we currently have (if I'm wrong please let me know.) What you propose would mean fully refactoring all renderers to do paddings using margins (plus paddings for those container that have a background), which:

  • Is way more complicated (not to mention less elegant)
  • Would require a ton of work

@andrewleader andrewleader changed the title Proposal: Margin property on containers Proposal: marginFromParent property Oct 18, 2018
@andrewleader
Copy link
Contributor Author

Closing as we decided not to take this approach for 1.2 and instead went with bleed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants