-
Notifications
You must be signed in to change notification settings - Fork 46
Adds WhatsNew to Settings, displaying content from a Markdown file in project #172
base: dev
Are you sure you want to change the base?
Conversation
…xed a SwiftLint trailing_newline warning in AWSTweet+Extension. Added function to SettingsViewModel to get unformatted string from bundle. This now displays in WhatsNewView. Added second function to create AttributedString, changed Markdown file formatting, now creating and using AttributedString in WhatsNewView
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.
Good work! I have several changes that I'd like, thanks!
ENABLE_PREVIEWS = YES; | ||
INFOPLIST_FILE = "brain-marks/Info.plist"; | ||
IPHONEOS_DEPLOYMENT_TARGET = 14.0; | ||
IPHONEOS_DEPLOYMENT_TARGET = 15.0; |
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.
Why is the deployment target changed to iOS 15.0
?
Section(header: Text("Updates")) { | ||
NavigationLink { | ||
WhatsNewView(viewModel: viewModel) | ||
} label: { | ||
HStack { | ||
Text("What's New") | ||
} | ||
} | ||
} |
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.
Can you fix the indentation here?
The NavigationLink
shouldn't be indented this far
Also the HStack
isn't needed
import SwiftUI | ||
|
||
struct WhatsNewView: View { | ||
@StateObject var viewModel: SettingsViewModel |
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 should be an ObservedObject
rather than a StateObject
var body: some View { | ||
let plainString = viewModel.getStringFromBundle() | ||
let attributedString = viewModel.createAttributedString(plainString: plainString) | ||
Text(attributedString) | ||
.padding() | ||
Spacer() | ||
} |
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.
Can you put this into a ScrollView
? That will help accommodate as the file gets bigger and with dynamic type. Here's an interesting article!
A better way to this would be to have a function called createWhatsNewAttributedString
that uses the function getStringFromBundle
rather than needing 2 variables in the View
This also makes it so you can make getStringFromBundle
a private function
Lastly instead of returning a blank string, you could return an error message saying like "Problem with file" idk haha something useful, otherwise an empty string is weird
struct WhatsNewView: View { | ||
@StateObject var viewModel: SettingsViewModel | ||
|
||
var body: some View { |
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.
Can you also add a navigation title to this screen, just "What's New" is fine!
@@ -0,0 +1,5 @@ | |||
**v1.2** |
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 is fine, if you don't want to change it, but I'd prefer for the version numbers to be like this # v1.2
, which would show them as a heading rather than just bold
.resume() | ||
} | ||
|
||
func getStringFromBundle() -> String { |
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 would rename this to be more specific that this is only for getting the whatsnew
file, maybe something like getWhatsNewFromBundle()
??
Closes #163
What it Does
Adds a new UPDATES section to the Settings view, with so far just a single row, What's New. When tapped, this leads to a view that displays a Markdown file showing the app changelog.
How I Tested
Notes
AttributedString
does not appear to support headers in Markdown, these were reformatted as emboldened text instead.trailing_newline
warning.Screenshot