[.NET][WPF][HTML] Fix toggle visibility spacing #2767
Conversation
| ColumnDefinition columnDefinition = new ColumnDefinition() { Width = new GridLength(1, GridUnitType.Star) }; | ||
| tag.NotAutoWidthColumnDefinition = columnDefinition; | ||
| tag.ViewIndex = uiColumnSet.ColumnDefinitions.Count; | ||
| uiColumnSet.ColumnDefinitions.Add(columnDefinition); |
There was a problem hiding this comment.
can't this more cleanly be written by generating the ColumnDefinition for each situation, and then adding it in one spot? #Closed
|
|
||
| const parent = targetElement.parentNode; | ||
| var isFirstElement = true; | ||
| for(var k = 0; k < parent.childNodes.length; k++){{ |
There was a problem hiding this comment.
mix of tabs and spaces #Closed
| @@ -279,17 +279,68 @@ static int Main(string[] args) | |||
| // The way to discern between checkbox elements and inline-actions is that inline-actions contain a textinput | |||
| var isCheckBoxElement = ((targetElementsInDocument.length > 1) && !(targetElement.className.includes('ac-textinput'))); | |||
|
|
|||
| const targetSeparatorId = targetElement.dataset.acSeparatorid; | |||
There was a problem hiding this comment.
[](start = 86, length = 6)
unnecessary trailing tabs here #Closed
|
|
||
| public AdaptiveSpacing Spacing { get; set; } = AdaptiveSpacing.None; | ||
|
|
||
| public Grid ElementContainer { get; set; } |
There was a problem hiding this comment.
ElementContainer [](start = 20, length = 16)
rename to ParentElementContainer #Resolved
There was a problem hiding this comment.
|
|
||
| public ColumnDefinition NotAutoWidthColumnDefinition { get; set; } = null; | ||
|
|
||
| public RowDefinition NotAutoHeightRowDefinition { get; set; } = null; |
There was a problem hiding this comment.
rename to ColumnDefinition and RowDefinition and store auto by default #Resolved
| @@ -83,9 +82,12 @@ public static void AddContainerElements(Grid uiContainer, IList<AdaptiveElement> | |||
| FrameworkElement uiElement = context.Render(cardElement); | |||
| if (uiElement != null) | |||
| { | |||
| TagContent tag = new TagContent(AdaptiveSpacing.None, uiContainer); | |||
There was a problem hiding this comment.
add this as a last else #Resolved
| } | ||
| else | ||
| { | ||
| RowDefinition rowDefinition = new RowDefinition() { Height = new GridLength(1, GridUnitType.Star) }; |
There was a problem hiding this comment.
RowDefinition [](start = 24, length = 13)
add comment for clarification on what tag information is being stored here and why #Resolved
| uiColumnSet.ColumnDefinitions.Add(new ColumnDefinition() { Width = GridLength.Auto }); | ||
| { | ||
| columnDefinition = new ColumnDefinition() { Width = GridLength.Auto }; | ||
|
|
There was a problem hiding this comment.
nit: extra line
#Resolved
| { | ||
| columnDefinition = new ColumnDefinition() { Width = GridLength.Auto }; | ||
| } | ||
|
|
There was a problem hiding this comment.
nit: extra line #Resolved
| @@ -286,24 +286,164 @@ public FontColorConfig GetForegroundColors(AdaptiveTextColor textColor) | |||
| } | |||
| } | |||
|
|
|||
| private TagContent GetTagContent(FrameworkElement element) | |||
| { | |||
| if (element != null) | |||
There was a problem hiding this comment.
consolidate this ifs as they are checked left to right #Resolved
| return null; | ||
| } | ||
|
|
||
| public void SetVisibility(FrameworkElement element, bool isVisible, TagContent tagContent) |
There was a problem hiding this comment.
isVisible [](start = 65, length = 9)
rename to desiredVisibility #Resolved
| } | ||
|
|
||
| // Trying to set the same rowDefinition twice to the same element is not valid, so we have to make a check first | ||
| if (!(elementIsCurrentlyVisible && isVisible)) |
There was a problem hiding this comment.
isVisible [](start = 51, length = 9)
move this check so the column and row Definitions are not created
#Resolved
| // if we read something with the format {"elementId": <id>", "isVisible": true} or we just read the id and the element is not visible | ||
| if ((targetElement.IsVisible.HasValue && targetElement.IsVisible.Value) || (!targetElement.IsVisible.HasValue && visibility != Visibility.Visible)) | ||
| // otherwise if we read something with the format {"elementId": <id>", "isVisible": false} or we just read the id and the element is visible |
There was a problem hiding this comment.
wrap lines to 120ish characters so they are not extra long #Resolved
|
|
||
| SetVisibility(elementFrameworkElement, newVisibility, tagContent); | ||
|
|
||
| if (tagContent != null) |
There was a problem hiding this comment.
check just with tagContent for null check #ByDesign
There was a problem hiding this comment.
C# doesn't allow null checks with only the variable name as C or C++ do
In reply to: 280220168 [](ancestors = 280220168)
| // if we read something with the format {"elementId": <id>", "isVisible": true} or we just read the id and the element is not visible | ||
| if ((targetElement.IsVisible.HasValue && targetElement.IsVisible.Value) || (!targetElement.IsVisible.HasValue && visibility != Visibility.Visible)) | ||
| // otherwise if we read something with the format {"elementId": <id>", "isVisible": false} or we just read the id and the element is visible | ||
| bool newVisibility = (targetElement.IsVisible.HasValue && targetElement.IsVisible.Value) || (!targetElement.IsVisible.HasValue && !isCurrentlyVisible); |
There was a problem hiding this comment.
split conditional and comment #Resolved
| } | ||
|
|
||
| /// <summary> | ||
| /// Elements are adde to the container in two ways: if the height is auto or the inserted element is a container, then the element |
| } | ||
|
|
||
| /// <summary> | ||
| /// Elements are adde to the container in two ways: if the height is auto or the inserted element is a container, then the element |
There was a problem hiding this comment.
Regenerate and make the summary clearer #Resolved
| } | ||
|
|
||
| Thickness renderedMargin = renderedElement.Margin; | ||
| renderedElement.Margin = new Thickness(renderedMargin.Left, |
There was a problem hiding this comment.
change the spacing form being a margin to being a grid
Do this here or add a note on Bug #2816 for bleed #Resolved
| "height": "stretch", | ||
| "version": "1.0", | ||
| "body": [ | ||
| { |
There was a problem hiding this comment.
run the UWP test app #Resolved
| @@ -0,0 +1,42 @@ | |||
| using System; | |||
There was a problem hiding this comment.
add copyright banner #Resolved
| foreach (var targetElement in toggleVisibilityAction.TargetElements) | ||
| { | ||
| // If the string is not empty, append a comma in preparation to add the new target element | ||
| if (!String.IsNullOrWhiteSpace(targetElements)) |
There was a problem hiding this comment.
change this to targetElements.IsNullOrWhiteSpace #Resolved
| // if it's visible and it's the first element, hide the separator | ||
| if (isFirstElement) | ||
| { | ||
| if (uiSeparator != null) |
There was a problem hiding this comment.
change null checks from "element != null" to "element" #Resolved
There was a problem hiding this comment.
| } | ||
| } | ||
|
|
||
| if (!String.IsNullOrEmpty(cardElement.Id)) |
There was a problem hiding this comment.
use IsNullOrWhiteSpace rather than IsNullOrEmpty #Resolved
There was a problem hiding this comment.
btw wrt our offline discussion, you can't call isNullOrWhitespace on the instance/object directly unless there is an extension method defined (probably would be neat) so incorrect on that front also; btw prefer using the string alias instead of the String class name generally but for now consistency with rest of the code base is fine!
In reply to: 280228354 [](ancestors = 280228354)
| } | ||
|
|
||
| // if explicit image size is not used, use Adaptive Image size | ||
| // if explicit image size is not used, use Adpative Image size |
There was a problem hiding this comment.
Adpative [](start = 55, length = 8)
introduced typo here #Resolved
|
@almedina-ms anything blocking this review? |
|
just missing approval In reply to: 490986918 [](ancestors = 490986918) |
|
PR also fixes #2890 |
Fixes #2604
Also fixes #2890
The big problem is that toggle visibility doesn't interact correctly with elements that have spacing between them, so when the elements are hidden, their separators are not hidden (the same behaviour happens when the element is originally not visible).
How verified
Two test files were modified for testing the correct behavior and, for testing purposes, the separator color was changed so the change would be more visible
WPF
For WPF the spacings are margins at the top so that value must be modified when hiding and showing
Hide the separator for the first visible element on card creation
Hide the separator for the first visible element on toggle action
Initial state
State after the first element (Image) was hidden
State after the first column has been hidden
HTML
Hide the separator for the first visible element on card creation
Hide the separator for the first visible element on toggle action
Initial state
State after the first element (Image) was hidden
State after the first column has been hidden
