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

added handler for column width in pixels #1746

Merged
merged 7 commits into from
Aug 3, 2018

Conversation

tuanh118
Copy link
Contributor

@tuanh118 tuanh118 commented Jul 31, 2018

Not sure if double unit makes sense for pixel (e.g., 50.3px) and also for weighted proportion (already in use)

For testing, you can use the payload below. I noticed the payload provided in the epic does not have "type": "Column" anywhere and the .NET renderer throws an error for it. Is that something missing from the .NET renderer?

{
  "$schema": "http://adaptivecards.io/schemas/adaptive-card.json",
  "type": "AdaptiveCard",
  "version": "1.0",
  "body": [
    {
      "type": "ColumnSet",
      "columns": [
        {
          "type": "Column",
          "width": "50px",
          "items": [
            {
              "type": "Image",
              "url": "http://3.bp.blogspot.com/-Xo0EuTNYNQg/UEI1zqGDUTI/AAAAAAAAAYE/PLYx5H4J4-k/s1600/smiley+face+super+happy.jpg",
              "size": "stretch"
            }
          ]
        },
        {
          "type": "Column",
          "width": "stretch",
          "items": [
            {
              "type": "TextBlock",
              "text": "This card has two ColumnSets on top of each other. In each, the left column is explicitly sized to be 50 pixels wide.",
              "wrap": true
            }
          ]
        }
      ]
    },
    {
      "type": "ColumnSet",
      "columns": [
        {
          "type": "Column",
          "width": "50px"
        },
        {
          "type": "Column",
          "width": "stretch",
          "items": [
            {
              "type": "TextBlock",
              "text": "In this second ColumnSet, columns align perfectly even though there is nothing in the left column.",
              "wrap": true
            }
          ]
        }
      ]
    }
  ]
}

@matthidinger
Copy link
Member

At some point we stopped requiring a type on Column, the .NET Renderer is probably out of date there. I think int makes sense for pixel unit, not sure where fraction of a pixel applies?

@khouzam
Copy link

khouzam commented Aug 1, 2018

We need to make sure that we're handling parsing of the units in the same way on all renderers. @paulcam206 noticed that also and is looking at fixing it.

Fractional pixels can make sense since we're using logical pixels, but the width is exposed as an int in the shared model. In terms of functionality, fractional pixels don't really make a that much of a difference (until we add transforms and scales then could really affect the widths). My main concern here is consistency in how we parse the value having 23.5px be valid in one renderer but not in others will make for a bad sharing experience.

@paulcam206
Copy link
Member

I have a change in #1750 that deals with fractional pixels. FWIW, the visualizer supports (or at least allows) fractional pixels.

@tuanh118
Copy link
Contributor Author

tuanh118 commented Aug 1, 2018

So the consensus is we allow "23.5px" and also render exactly 23.5px right?

@paulcam206
Copy link
Member

Well, as @khouzam noted, the shared model exposes the width as an unsigned int, so we can't currently have non-integral sizes on the rendering side. It's something we might want to pursue in the future, but can't today without a breaking change to the object model.

@tuanh118
Copy link
Contributor Author

tuanh118 commented Aug 1, 2018

But since the TypeScript visualizer is allowing fractional pixels, we should at least allow it in other renderers right? So that a card is not rendered differently in different renderers?

@paulcam206
Copy link
Member

the way I've addressed it in #1750 is that the parser will accept fractional values, but still only exposes integral values through the object model. let's have a chat later about what to do.

one thing I don't know is whether the typescript visualizer is actually using fractional units, or just snapping to the nearest integer.

@tuanh118
Copy link
Contributor Author

tuanh118 commented Aug 1, 2018

https://github.com/Microsoft/AdaptiveCards/blob/master/source/nodejs/adaptivecards/src/card-elements.ts#L101
It's using the int part of the number. For example, 11.8 -> 11, -11.8 -> -11

@paulcam206
Copy link
Member

I think this lines up with what std::stoi does, so both would be in agreement.

@tuanh118
Copy link
Contributor Author

tuanh118 commented Aug 1, 2018

I just added final modifications so that it aligns with the TypeScript renderer: allow fractional values for pixel and truncates it to use only its integral part.
Also added a check to handle negative width

Copy link

@khouzam khouzam left a comment

Choose a reason for hiding this comment

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

:shipit:

@tuanh118 tuanh118 merged commit 9e8e891 into master Aug 3, 2018
@paulcam206 paulcam206 deleted the tupha/explicit-column-pixel-width-dotnet branch August 31, 2019 19:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants