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

minHeight property on containers #2293

Closed
7 tasks done
andrewleader opened this issue Jan 11, 2019 · 9 comments
Closed
7 tasks done

minHeight property on containers #2293

andrewleader opened this issue Jan 11, 2019 · 9 comments

Comments

@andrewleader
Copy link
Contributor

andrewleader commented Jan 11, 2019

Release Renderer status Tasks status
1.2 ✔️ .NET (#2618)
✔️ Android (#2619)
✔️ iOS (#2620)
✔️ TS (#2621)
✔️ UWP (#2622)
✔️ Shared (#2616)
✔️ Designer (#2617)

Solves requests

Summary

This proposal is an alternative way of solving the above two requests, written up so we can consider both options. Containers can already almost achieve the two above requests except for one thing... you can't set a height on them.

Schema

New property on the following elements...

  • Container
  • ColumnSet
  • Column
  • AdaptiveCard
Property Type Required Description
minHeight null (default) or px false Specifies the minimum height of the container.

Example

To achieve the following, where the image is full width but has a fixed height...

image

I would add a container with a background image, and set the minimum height of the container to 100px (so it's my exact height).

        {
            "type": "Container",
            "backgroundImage": {
              "url": "https://picsum.photos/300/400?image=882"
            },
            "minHeight": "100px"
        }

Now, what if I want the image to go to the edge of the card? I don't want that padding around the edges?

image

I would simply set bleed=true, and that'll remove the padding.

        {
            "type": "Container",
            "backgroundImage": {
              "url": "https://picsum.photos/300/400?image=882"
            },
            "minHeight": "100px",
            "bleed": true
        }

Host Config

None.

Down-level impact

High. Image won't appear at all (since height on container is ignored down-level, and therefore the container won't have any height).

Host burden

None.

Auto-generated task status

  • Shared
  • Designer
  • .NET
  • Android
  • iOS
  • TS
  • UWP
@andrewleader andrewleader changed the title Proposal: height property on containers Spec draft: height property on containers Jan 12, 2019
@andrewleader
Copy link
Contributor Author

Proposal approved!

@dclaux
Copy link
Member

dclaux commented Jan 13, 2019

I have implemented the feature in the renderer and designer.Allowing for setting height absolutely feels weird in the designer; you can get to super skinny containers that are hard/almost impossible to interact with, and as you add more elements than the container can hold the experience feels broken as well.

I think we should make "height": "50px" mean "minimum height is 50px" (that's what I'm going to do for now), or better even introduce a minHeight property instead.

@paulcam206
Copy link
Member

consensus: build minimum height logic and call it minHeight

@mdtauk
Copy link

mdtauk commented Jan 14, 2019

consensus: build minimum height logic and call it minHeight

If you have a Minimum height, why not Maximum with scrolling for bits that overflow or extended out of a container?

@dclaux
Copy link
Member

dclaux commented Jan 14, 2019

@mdtauk I'll return the question: why do what you suggest? What scenario does it support? Not saying I'm opposed, but it doesn't seem necessary.

@andrewleader andrewleader changed the title Spec draft: height property on containers Spec draft: minHeight property on containers Jan 14, 2019
@mdtauk
Copy link

mdtauk commented Jan 14, 2019

@mdtauk I'll return the question: why do what you suggest? What scenario does it support? Not saying I'm opposed, but it doesn't seem necessary.

The very fact you would have a Minimum Width property, would make people expect to be a Maximum. Why only constrain in one direction?

@dclaux
Copy link
Member

dclaux commented Jan 14, 2019

Because there is a reason to have a minimum height (namely what this spec describes) while we have not identified a reason why having a maximum height would be desirable. It's always easier to add new features as necessary rather than introduce it without concrete evidence it's useful and have to live with it forever.

Do you have a concrete scenario in mind that would need a maximum height?

@andrewleader
Copy link
Contributor Author

andrewleader commented Apr 8, 2019

We updated the spec so that it's only on containers. This was because minHeight on images behaved oddly... we could make minHeight on images behave as you'd expect (same as setting height property), but since we don't know of any current use cases for minHeight on images or other elements, we're avoiding adding it to those for now.

@andrewleader
Copy link
Contributor Author

Updating spec to reflect that we also added it to AdaptiveCard

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

6 participants