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

Spec for column vs image size #4887

Merged
merged 8 commits into from
Nov 5, 2020
Merged

Conversation

almedina-ms
Copy link
Contributor

@almedina-ms almedina-ms commented Oct 6, 2020

Related Issue

Please use one of the well-known github fixes keywords to reference
the issues fixed with this PR (eg Fixes #). If an issue doesn't yet exist please create one if reasonable to aid
in issue tracking.

NOTE: For multiple issues resolved by this PR use the corresponding keywords every time in a comma-delimited list per the reference
page above.

Description

Create a descriptive title and provide all of the relevant details in the description.
This information lives with the code and will help future readers (including yourself) and will serve as documentation.

At the very least please describe the issue you are addressing, and what your fix entails.

How Verified

How you verified the fix, including one or all of the following:

  1. New unit tests that were added if any. If none were added please add a quick line explaining why not.
  2. Existing relevant unit/regression tests that you ran
  3. Manual scenario verification if any; Do include .gif's or screenshots of the testing you performed here if you think that it
    will aid in code reviews or corresponding fixes on other platforms for eg.
Microsoft Reviewers: Open in CodeFlow

@shalinijoshi19
Copy link
Member

Related Issue

Please use one of the well-known github fixes keywords to reference
the issues fixed with this PR (eg Fixes #). If an issue doesn't yet exist please create one if reasonable to aid
in issue tracking.

NOTE: For multiple issues resolved by this PR use the corresponding keywords every time in a comma-delimited list per the reference
page above.

Description

Create a descriptive title and provide all of the relevant details in the description.
This information lives with the code and will help future readers (including yourself) and will serve as documentation.

At the very least please describe the issue you are addressing, and what your fix entails.

How Verified

How you verified the fix, including one or all of the following:

  1. New unit tests that were added if any. If none were added please add a quick line explaining why not.
  2. Existing relevant unit/regression tests that you ran
  3. Manual scenario verification if any; Do include .gif's or screenshots of the testing you performed here if you think that it
    will aid in code reviews or corresponding fixes on other platforms for eg.
Microsoft Reviewers: Open in CodeFlow

@almedina-ms can you update the description on this PR and remove the template placeholder text? Thanks


### Current problem

As part of the solution to the Column width behaviour it was thought that images could be used as a reference to provide the minimum size needed to render an `auto` width column. This asseveration is not correct, as some of our scenario cards don't follow that principle, take for example the [FlightDetails card](), in this card we have an image whose `width` is `1000px`, if we were to honour the measurements given by the card author we would get a card where this long image occupies most of the width of the column set as can be seen in the views generated by the iOS and the Android renderers.
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for trying to add context here. But can you also link to the "Column width behavior" issue that you are referring to here for clarification? Thanks

Copy link
Member

Choose a reason for hiding this comment

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

Is it #4722 ?


### Current problem

As part of the solution to the Column width behaviour it was thought that images could be used as a reference to provide the minimum size needed to render an `auto` width column. This asseveration is not correct, as some of our scenario cards don't follow that principle, take for example the [FlightDetails card](), in this card we have an image whose `width` is `1000px`, if we were to honour the measurements given by the card author we would get a card where this long image occupies most of the width of the column set as can be seen in the views generated by the iOS and the Android renderers.
Copy link
Member

Choose a reason for hiding this comment

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

NitNit: :) I had to look up the dictionary for that one :) - Could we use 'assumption' here?


### Current problem

As part of the solution to the Column width behaviour it was thought that images could be used as a reference to provide the minimum size needed to render an `auto` width column. This asseveration is not correct, as some of our scenario cards don't follow that principle, take for example the [FlightDetails card](), in this card we have an image whose `width` is `1000px`, if we were to honour the measurements given by the card author we would get a card where this long image occupies most of the width of the column set as can be seen in the views generated by the iOS and the Android renderers.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
As part of the solution to the Column width behaviour it was thought that images could be used as a reference to provide the minimum size needed to render an `auto` width column. This asseveration is not correct, as some of our scenario cards don't follow that principle, take for example the [FlightDetails card](), in this card we have an image whose `width` is `1000px`, if we were to honour the measurements given by the card author we would get a card where this long image occupies most of the width of the column set as can be seen in the views generated by the iOS and the Android renderers.
As part of the solution to the Column width behavior it was thought that images could be used as a reference to provide the minimum size needed to render an `auto` width column. This assumption is not correct as some of our scenario cards don't follow that principle. Take for example the [FlightDetails card](), where we have an image whose `width` is `1000px`; If we were to honor the measurements given by the card author we would get a card where this long image occupies most of the width of the column set as can be seen in the views generated by the iOS and the Android renderers.


| Rendered view | Schema
| --- | --- |
| <img src="assets/ImageColumn/BadFlightDetails.png" width="800px"> | <img src="assets/ImageColumn/BadRenderSchema.png" width="800px"> |
Copy link
Member

Choose a reason for hiding this comment

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

This results in a 404.. Does this need to include the relative path here vis "../assets"?

Copy link
Member

@jwoo-msft jwoo-msft left a comment

Choose a reason for hiding this comment

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

Hey,

Excellent work! This spec will really help. In case we forget to check-this in. I'm adding the approval, so this PR will get check-in.

@ghost ghost added the no-recent-activity label Oct 13, 2020
@ghost ghost assigned matthidinger Oct 13, 2020
@ghost
Copy link

ghost commented Oct 13, 2020

Hi @almedina-ms. This non-spec pull request has had no recent activity for the past 5 days . Please take the necessary actions (review, address feedback or commit if reviewed already) to move this along.

@ghost ghost removed the no-recent-activity label Nov 5, 2020
@ghost
Copy link

ghost commented Nov 5, 2020

Hi @almedina-ms; Thanks for taking action on your previously stale pull request. Resetting staleness.

@almedina-ms almedina-ms merged commit 25554e2 into main Nov 5, 2020
@almedina-ms almedina-ms deleted the user/almedina-ms/Spec_ImageVColumn branch November 5, 2020 21:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants