Forms navigation XML docs #2475
Conversation
|
@dansiegel I don't think these changes caused the end to end Uno UITests to fail. Does this need to be fixed before merging? |
|
No your changes wouldn't have caused anything to fail... I'll review this when I get a chance |
| public interface INavigationResult | ||
| { | ||
| /// <summary> | ||
| /// <see langword="true"/> if the navigation was successful, <see langword="false"/> otherwise. |
There was a problem hiding this comment.
May be false with a null exception if CanNavigate returned false
There was a problem hiding this comment.
Not sure I'm following your correction here. If CanNavigate returns false, then that is not a successful navigation so the summary for Success still sounds correct.
The summary for INavigationResult isn't correct however and I think it'd be best to update it to be a bit more generic, something like "Provides navigation results indicating if the navigation was successful and any exceptions caught".
What do you think?
There was a problem hiding this comment.
I went ahead and made those changes. In the descriptions of INavigationResult it now doesn't imply there is an exception when success is false.
There was a problem hiding this comment.
@dansiegel Are you okay with these changes, can we get this PR merged?
The wording before suggested that if the navigation request was unsuccessful there would be an exception captured; however, this is not the case when navigation is attempted and CanNavigate is false. In this case, Success would be false and Exception would be null. The wording now does not make that suggestion.
Adam--
force-pushed
the
forms-navigation-docs
branch
from
c244107
to
f9cc1c1
Compare
|
I'm closing this PR since it's growing quite old. Maybe in the future we can get some docs in. |
Description of Change
Addresses part of #1978, Add Missing XML Docs, adding documentation for all public members and classes within Prism.Forms Navigation.
Bugs Fixed
None
API Changes
None
Added:
None
Changed:
None
Behavioral Changes
None
PR Checklist
Has tests (if omitted, state reason in description)