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

[Docs] Added pending inline comments to Page class #20933

Open
wants to merge 30 commits into
base: main
Choose a base branch
from

Conversation

jsuarezruiz
Copy link
Contributor

Description of Change

Added pending inline comments to Page class.

@jsuarezruiz jsuarezruiz added area-docs Conceptual docs, API docs, Samples t/housekeeping ♻︎ labels Mar 1, 2024
@jsuarezruiz jsuarezruiz requested a review from a team as a code owner March 1, 2024 08:46
Copy link
Contributor

@jknaudt21 jknaudt21 left a comment

Choose a reason for hiding this comment

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

I have similar comments as in: #20912

Additionally, I would say to keep an eye out to not to directly mention type that a method returns in the summary if possible and instead opt to use more general language instead.

jsuarezruiz and others added 2 commits March 5, 2024 15:39
Co-authored-by: Juan Diego Herrera <juherrera@microsoft.com>
public static void SetPrefersHomeIndicatorAutoHidden(BindableObject element, bool value)
{
element.SetValue(PrefersHomeIndicatorAutoHiddenProperty, value);
}

/// <include file="../../../../docs/Microsoft.Maui.Controls.PlatformConfiguration.iOSSpecific/Page.xml" path="//Member[@MemberName='PrefersHomeIndicatorAutoHidden']/Docs/*" />
/// <summary>
/// Gets a Boolean that indicates whether is allowed to hide the visual indicator for returning to the Home Screen.
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar comment as SetPrefersHomeIndicatorAutoHidden

public static bool PrefersHomeIndicatorAutoHidden(this IPlatformElementConfiguration<iOS, FormsElement> config)
{
return GetPrefersHomeIndicatorAutoHidden(config.Element);
}

/// <include file="../../../../docs/Microsoft.Maui.Controls.PlatformConfiguration.iOSSpecific/Page.xml" path="//Member[@MemberName='SetPrefersHomeIndicatorAutoHidden'][2]/Docs/*" />
/// <summary>
/// Sets a Boolean that indicates whether is allowed to hide the visual indicator for returning to the Home Screen.
Copy link
Contributor

Choose a reason for hiding this comment

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

How is this property different to SetPrefersHomeIndicatorAutoHidden?

Copy link
Contributor

Choose a reason for hiding this comment

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

This file should be entirely deleted

Copy link
Contributor

Choose a reason for hiding this comment

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

This file should be entirely deleted

jknaudt21
jknaudt21 previously approved these changes May 7, 2024
jknaudt21
jknaudt21 previously approved these changes May 8, 2024
@@ -9,35 +9,56 @@ namespace Microsoft.Maui.Controls.PlatformConfiguration.WindowsSpecific
{
using FormsElement = Maui.Controls.Page;

/// <include file="../../../../docs/Microsoft.Maui.Controls.PlatformConfiguration.WindowsSpecific/Page.xml" path="Type[@FullName='Microsoft.Maui.Controls.PlatformConfiguration.WindowsSpecific.Page']/Docs/*" />
/// <summary>
/// The page instance that Microsoft.Maui.Controls created on the Windows platform.
Copy link
Member

Choose a reason for hiding this comment

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

This isn't really correct

This is just an extension class for setting platform specifics

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

public static ToolbarPlacement GetToolbarPlacement(BindableObject element)
{
return (ToolbarPlacement)element.GetValue(ToolbarPlacementProperty);
}

/// <include file="../../../../docs/Microsoft.Maui.Controls.PlatformConfiguration.WindowsSpecific/Page.xml" path="//Member[@MemberName='SetToolbarPlacement'][1]/Docs/*" />
/// <summary>
/// Sets a value that controls the placement of the toolbar.
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't actually do anything right now. Is that useful to note ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

public static void MapToolbarPlacement(IToolbarHandler arg1, Toolbar arg2)

Not implemented

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How should we manage this case?

  • Include some notes in the docs?
  • Implement it?
  • Deprecate/remove it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think there are 2 approaches here:

  1. We can make it invisible from the IDE by addding [EditorBrowsable(EditorBrowsableState.Never)]
  2. Adding a remark saying that this functionality hasn't been implemeted yet.

The ideal scenario would be implementing the missing functionality, but that may go beyond the scop of this PR.

@davidbritch what would be your recommended approach here?

Copy link
Contributor

Choose a reason for hiding this comment

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

If the API does nothing right now I'd hide it in the IDE.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jsuarezruiz. With this change I think we should be good to merge. I'd hate for this PR to go stale

public static ToolbarPlacement GetToolbarPlacement(BindableObject element)
{
return (ToolbarPlacement)element.GetValue(ToolbarPlacementProperty);
}

/// <include file="../../../../docs/Microsoft.Maui.Controls.PlatformConfiguration.WindowsSpecific/Page.xml" path="//Member[@MemberName='SetToolbarPlacement'][1]/Docs/*" />
/// <summary>
/// Sets a value that controls the placement of the toolbar.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think there are 2 approaches here:

  1. We can make it invisible from the IDE by addding [EditorBrowsable(EditorBrowsableState.Never)]
  2. Adding a remark saying that this functionality hasn't been implemeted yet.

The ideal scenario would be implementing the missing functionality, but that may go beyond the scop of this PR.

@davidbritch what would be your recommended approach here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-docs Conceptual docs, API docs, Samples t/housekeeping ♻︎
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants