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.

Sign up for GitHub

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

Jump to bottom

[Android] Fix Toggle visibility to dissappear separation #2742

Merged
merged 14 commits into from
May 9, 2019
Merged

[Android] Fix Toggle visibility to dissappear separation #2742

merged 14 commits into from
May 9, 2019

Conversation

almedina-ms
Copy link
Contributor

@almedina-ms almedina-ms commented Apr 22, 2019 •
edited
Loading

Adds extra data to the TagContent object to be able to hide the separators whenever the element is hidden (or show them when the element re appears)

Also fixes the case where ther first element is deleted and the second element has to hide its separator (and all related cases)

Fix for this: #2605

How verified

The mobile app was run and this are the results

Case 1

Top element disappears and second element should hide its separator
Screenshot_20190422-163543

When the image is hidden, the textblock separator hides

Screenshot_20190422-163549

When the image reappears, the textblock separator also reappears

Screenshot_20190422-163826

Case 2

First column in columnset disappears, so the second column must hide its separator

Screenshot_20190422-163554

When the first column disappears, the column separator hides

Screenshot_20190422-163558

When the first column is shown, so does the column 2 separator

Screenshot_20190422-163821

paulcam206
paulcam206 reviewed Apr 23, 2019
View reviewed changes
...android/adaptivecards/src/main/java/io/adaptivecards/renderer/BaseActionElementRenderer.java Outdated

if (tag != null && tag instanceof TagContent)
{
TagContent tagContent = (TagContent)tag;
Copy link
Member

@paulcam206 paulcam206 Apr 23, 2019 •
edited by almedina-ms
Loading

Choose a reason for hiding this comment

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

I know it's kind of small, but it might be nice to have a little helper function that takes a View and gives you its TagContent. Feel free to say no or add it in a future checkin. :) #Resolved

paulcam206
paulcam206 approved these changes Apr 23, 2019
View reviewed changes
Copy link
Member

@paulcam206 paulcam206 left a comment

Choose a reason for hiding this comment

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

:shipit:

@RebeccaAnne
Copy link
Contributor

RebeccaAnne commented Apr 24, 2019 •
edited
Loading

Could you add a link to the issue tracking this?

Also, do we have separate bugs tracking bleed changes based on toggle? #Resolved

@almedina-ms
Copy link
Contributor Author

I added the link but the issues track both changes (bleed & separator)

In reply to: 486355510 [](ancestors = 486355510)

shalinijoshi19
shalinijoshi19 reviewed Apr 25, 2019
View reviewed changes
...android/adaptivecards/src/main/java/io/adaptivecards/renderer/BaseActionElementRenderer.java Outdated
@@ -110,13 +113,52 @@ private void populateViewsDictionary()
String elementId = target.GetElementId();

View foundView = rootView.findViewWithTag(new TagContent(elementId));
if(foundView != null)
if (foundView != null)
Copy link
Member

Choose a reason for hiding this comment

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

if ( [](start = 15, length = 5)

Is this clang-formatted?

shalinijoshi19
shalinijoshi19 reviewed Apr 25, 2019
View reviewed changes
...android/adaptivecards/src/main/java/io/adaptivecards/renderer/BaseActionElementRenderer.java
private void resetSeparatorVisibilities(ViewGroup viewGroup)
{
boolean isFirstElement = true;
for (int i = 0; i < viewGroup.getChildCount(); ++i)
Copy link
Member

Choose a reason for hiding this comment

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

Nit: consider using the more compact foreach syntax

shalinijoshi19
shalinijoshi19 reviewed Apr 25, 2019
View reviewed changes
...android/adaptivecards/src/main/java/io/adaptivecards/renderer/BaseActionElementRenderer.java

if (tagContent != null)
{
if (!tagContent.IsSeparator() && element.getVisibility() == View.VISIBLE)
Copy link
Member

Choose a reason for hiding this comment

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

element.getVisibility() == View.VISIBLE [](start = 53, length = 39)

Nit: enclose in inner () for readability

shalinijoshi19
shalinijoshi19 reviewed Apr 25, 2019
View reviewed changes
...android/adaptivecards/src/main/java/io/adaptivecards/renderer/BaseActionElementRenderer.java Outdated
// If the visibility changes to not visible or the visibility toggles and the element is currently visible then the element will not be visible
// Otherwise it will be visible (default value)
if ((isVisible == IsVisible.IsVisibleFalse) ||
(isVisible == IsVisible.IsVisibleToggle && foundView.getVisibility() == View.VISIBLE))
Copy link
Member

Choose a reason for hiding this comment

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

ibleToggle & [](start = 57, length = 12)

nit: is this clang-formatted?

shalinijoshi19
shalinijoshi19 reviewed Apr 25, 2019
View reviewed changes
...android/adaptivecards/src/main/java/io/adaptivecards/renderer/BaseActionElementRenderer.java Outdated
@@ -140,28 +185,24 @@ public void onClick(View v)
View foundView = m_viewDictionary.get(elementId);
IsVisible isVisible = target.GetIsVisible();
Copy link
Member

Choose a reason for hiding this comment

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

IsVisible [](start = 24, length = 9)

we need a custom IsVisible type? why doesnt boolean suffice again?

@shalinijoshi19
Copy link
Member 

                horizontalLine ? 0 : spacingSize / 2 /* left */,

lots of magic numbers interspersed in the code base; Let;s make sure we address this eventually say in some Constants class or something

Refers to: source/android/adaptivecards/src/main/java/io/adaptivecards/renderer/BaseCardElementRenderer.java:91 in 567593e. [](commit_id = 567593e, deletion_comment = False)

shalinijoshi19
shalinijoshi19 reviewed Apr 25, 2019
View reviewed changes
...e/android/adaptivecards/src/main/java/io/adaptivecards/renderer/BaseCardElementRenderer.java
@@ -70,7 +73,7 @@ protected static void setSpacingAndSeparator(Context context,
if (viewGroup.getChildCount() <= 0)
{
//Do not add space to the first element of a viewgroup
return;
return null;
}
int spacingSize = Util.dpToPixels(context, getSpacingSize(spacing, hostConfig.GetSpacing()));
int separatorThickness = Util.dpToPixels(context, hostConfig.GetSeparator().getLineThickness());
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Function naming - generally prefer verb (convertDpToPixels or something)

shalinijoshi19
shalinijoshi19 reviewed Apr 25, 2019
View reviewed changes
...e/android/adaptivecards/src/main/java/io/adaptivecards/renderer/BaseCardElementRenderer.java
{
if (minHeight != 0)
{
view.setMinimumHeight(Util.dpToPixels(context, (int)minHeight));
Copy link
Member

Choose a reason for hiding this comment

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

int)minHeigh [](start = 60, length = 12)

why the need for different units and hence casting? (long vs int)

shalinijoshi19
shalinijoshi19 reviewed Apr 25, 2019
View reviewed changes
...roid/adaptivecards/src/main/java/io/adaptivecards/renderer/input/ChoiceSetInputRenderer.java
@@ -63,7 +63,9 @@ public static ChoiceSetInputRenderer getInstance()
public View renderCheckBoxSet(
RenderedAdaptiveCard renderedCard,
Context context,
ChoiceSetInput choiceSetInput)
ChoiceSetInput choiceSetInput,
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Inconsistent formatting of function parameters in teh source code (eg baseacationelementrenderer.java vs this file) . Is it an old school windows style each parameter on a new line? What is the clang formatting rules set to?

shalinijoshi19
shalinijoshi19 approved these changes Apr 25, 2019
View reviewed changes
Copy link
Member

@shalinijoshi19 shalinijoshi19 left a comment

Choose a reason for hiding this comment

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

:shipit:

@shalinijoshi19
Copy link
Member

Do we have separate bugs for other platforms for this issue?

@almedina-ms almedina-ms merged commit fc14a63 into master May 9, 2019
@almedina-ms almedina-ms deleted the user/almedina-ms/AndroidToggleVisibilityDissapperSeparation branch July 31, 2019 20:02
rankush pushed a commit to rankush/AdaptiveCards that referenced this pull request May 8, 2024
@almedina-ms
[Android] Fix Toggle visibility to dissappear separation (microsoft#2742
93d1941 
)

* Make separator creation method return the actual view

* Fix separator behaviour for hidden visibility

* Add method to avoid repeating object parsing

* Fix json

* Fix initial hidden state and toggle visibility for stretch items
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@paulcam206 paulcam206 paulcam206 approved these changes

@shalinijoshi19 shalinijoshi19 shalinijoshi19 approved these changes

@RebeccaAnne RebeccaAnne Awaiting requested review from RebeccaAnne

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@almedina-ms @RebeccaAnne @shalinijoshi19 @paulcam206