-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Conversation
There was a problem hiding this 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.
src/Controls/src/Core/PlatformConfiguration/WindowsSpecific/Page.cs
Outdated
Show resolved
Hide resolved
src/Controls/src/Core/PlatformConfiguration/iOSSpecific/Page.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Juan Diego Herrera <[email protected]>
src/Controls/src/Core/PlatformConfiguration/WindowsSpecific/Page.cs
Outdated
Show resolved
Hide resolved
src/Controls/src/Core/PlatformConfiguration/WindowsSpecific/Page.cs
Outdated
Show resolved
Hide resolved
src/Controls/src/Core/PlatformConfiguration/iOSSpecific/Page.cs
Outdated
Show resolved
Hide resolved
src/Controls/src/Core/PlatformConfiguration/iOSSpecific/Page.cs
Outdated
Show resolved
Hide resolved
src/Controls/src/Core/PlatformConfiguration/iOSSpecific/Page.cs
Outdated
Show resolved
Hide resolved
src/Controls/src/Core/PlatformConfiguration/iOSSpecific/Page.cs
Outdated
Show resolved
Hide resolved
src/Controls/src/Core/PlatformConfiguration/iOSSpecific/Page.cs
Outdated
Show resolved
Hide resolved
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. |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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?
Co-authored-by: Juan Diego Herrera <[email protected]>
Co-authored-by: Juan Diego Herrera <[email protected]>
Co-authored-by: Juan Diego Herrera <[email protected]>
Co-authored-by: Juan Diego Herrera <[email protected]>
Co-authored-by: Juan Diego Herrera <[email protected]>
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
src/Controls/src/Core/PlatformConfiguration/WindowsSpecific/Page.cs
Outdated
Show resolved
Hide resolved
src/Controls/src/Core/PlatformConfiguration/iOSSpecific/Page.cs
Outdated
Show resolved
Hide resolved
src/Controls/src/Core/PlatformConfiguration/iOSSpecific/Page.cs
Outdated
Show resolved
Hide resolved
src/Controls/src/Core/PlatformConfiguration/iOSSpecific/Page.cs
Outdated
Show resolved
Hide resolved
src/Controls/src/Core/PlatformConfiguration/iOSSpecific/Page.cs
Outdated
Show resolved
Hide resolved
src/Controls/src/Core/PlatformConfiguration/iOSSpecific/Page.cs
Outdated
Show resolved
Hide resolved
src/Controls/src/Core/PlatformConfiguration/iOSSpecific/Page.cs
Outdated
Show resolved
Hide resolved
8136301
to
2159676
Compare
@@ -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. |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
- We can make it invisible from the IDE by addding
[EditorBrowsable(EditorBrowsableState.Never)]
- 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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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:
- We can make it invisible from the IDE by addding
[EditorBrowsable(EditorBrowsableState.Never)]
- 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?
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
Description of Change
Added pending inline comments to Page class.