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

Merged
merged 30 commits into from
Jul 18, 2024
Merged

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.

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?

@PureWeen
Copy link
Member

/azp run

@PureWeen PureWeen enabled auto-merge (squash) July 17, 2024 14:07
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@PureWeen
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@PureWeen PureWeen merged commit 515c898 into main Jul 18, 2024
104 checks passed
@PureWeen PureWeen deleted the page-comments branch July 18, 2024 20:57
@samhouts samhouts added fixed-in-net9.0-nightly This may be available in a nightly release! fixed-in-8.0.80 and removed fixed-in-net9.0-nightly This may be available in a nightly release! labels Aug 2, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Sep 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-docs Conceptual docs, API docs, Samples fixed-in-8.0.80 fixed-in-net9.0-nightly This may be available in a nightly release! t/housekeeping ♻︎
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants