-
Notifications
You must be signed in to change notification settings - Fork 2
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
Enable webviews to change their title and icon url #583
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.
Looks good overall. Just one thing I see that should probably be addressed.
Reviewed 16 of 16 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @tjcouch-sil)
c-sharp/NetworkObjects/UsfmDataProvider.cs
line 100 at r1 (raw file):
private string GetUsxForBook(VerseRef vref) { XmlDocument usx = ConvertUsfmToUsx(GetUsfm(vref.BookNum, -1), vref.BookNum);
Minor nit: It seems like it would be a little cleaner to leave off the -1
since the intent is that the default argument covers the case of getting a book, not a chapter.
c-sharp/NetworkObjects/UsfmDataProvider.cs
line 130 at r1 (raw file):
/// <param name="chapterNum">The chapter for which to get USFM or -1 for the whole book</param> /// <returns>USFM</returns> private string GetUsfm(int bookNum, int chapterNum = -1)
It would be a little cleaner to use int?
defaulted to null
than int
defaulted to -1
. Then switch your check below about whether the value is set or not.
If you want to keep it as an int
instead of int?
, then you probably want to change the check from chapterNum == -1
to chaptuerNum >= 0
. If someone passes a value less than -1
I'm not sure what is intended to happen.
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.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @lyonsil)
c-sharp/NetworkObjects/UsfmDataProvider.cs
line 100 at r1 (raw file):
Previously, lyonsil (Matt Lyons) wrote…
Minor nit: It seems like it would be a little cleaner to leave off the
-1
since the intent is that the default argument covers the case of getting a book, not a chapter.
Done. Thanks!
c-sharp/NetworkObjects/UsfmDataProvider.cs
line 130 at r1 (raw file):
Previously, lyonsil (Matt Lyons) wrote…
It would be a little cleaner to use
int?
defaulted tonull
thanint
defaulted to-1
. Then switch your check below about whether the value is set or not.If you want to keep it as an
int
instead ofint?
, then you probably want to change the check fromchapterNum == -1
tochaptuerNum >= 0
. If someone passes a value less than-1
I'm not sure what is intended to happen.
Done. Thanks!
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.
Reviewed all commit messages.
Reviewable status: 15 of 16 files reviewed, 1 unresolved discussion (waiting on @tjcouch-sil)
c-sharp/NetworkObjects/UsfmDataProvider.cs
line 130 at r1 (raw file):
Previously, tjcouch-sil (TJ Couch) wrote…
Done. Thanks!
Thanks - once you update the param comment it should be good to go!
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.
Reviewable status: 15 of 16 files reviewed, 1 unresolved discussion (waiting on @lyonsil)
c-sharp/NetworkObjects/UsfmDataProvider.cs
line 130 at r1 (raw file):
Previously, lyonsil (Matt Lyons) wrote…
Thanks - once you update the param comment it should be good to go!
😖 done
25fc411
to
6044270
Compare
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.
Reviewed 4 of 4 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved
Refactored how React webviews work a bit so a few
WebViewProps
are now passed in for ease of access:Also added
getBookUsx
to the USFM data provider to get data for Ira.Subscribing to updates to the webview definition was split into #581 since that did not seem particularly valuable at this time.
Resolves #445
This change is