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

Onboarding Flow #1050

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Onboarding Flow #1050

wants to merge 13 commits into from

Conversation

garthvh
Copy link
Member

@garthvh garthvh commented Jan 11, 2025

What changed?

Why did it change?

How is this tested?

Screenshots/Videos (when applicable)

Checklist

  • My code adheres to the project's coding and style guidelines.
  • I have conducted a self-review of my code.
  • I have commented my code, particularly in complex areas.
  • I have made corresponding changes to the documentation.
  • I have tested the change to ensure that it works as intended.

@garthvh garthvh changed the base branch from 2.5.15 to 2.5.18 January 21, 2025 16:22
Base automatically changed from 2.5.18 to main February 2, 2025 20:08
/// The Title View
var title: some View {
VStack {
Text("Welcome to")
Copy link

@4np 4np Feb 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a note that the sentence Welcome to Meshtastic may have different grammar rules when translated into other languages so separating the sentence into two localized blocks has the potential to cause invalid grammar.

Also, it is best for all localized strings to have comments in order to provide context to translators.

  • the context of the view where a localized string is used may have impact on how the string should be translated as the context may affect grammar / or form of the translation (e.g. femine, masculine)
  • plural rules should be used when dealing with numbers as different languages have different requirements (for example, Russian requires more cases when dealing with plurals).
Suggested change
Text("Welcome to")
Text("Welcome to",
comment: "Onboarding Title: Welcome title, will precede the 'Meshtastic' brand (example: 'Welcome to \"Meshtastic\"')")

}

@ViewBuilder
func makeRow(
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makeRow ViewBuilder takes strings, but these are not localized. To ensure this will be localized these should be changed to use LocalizedStringResource.

Suggested change
func makeRow(
func makeRow(
iconSystemName: String,
title: LocalizedStringResource,
subtitle: LocalizedStringResource
) -> some View { .. }

var mqttView: some View {
VStack {
VStack {
Text("MQTT")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When strings don't need to be translated, you can use String(verbatim:):

Suggested change
Text("MQTT")
Text(verbatim: "MQTT")

@@ -12,7 +12,6 @@ class LocalNotificationManager {
// Step 1 Request Permissions for notifications
private func requestAuthorization() {
UNUserNotificationCenter.current().requestAuthorization(options: [.alert, .badge, .sound]) { granted, error in

if granted == true && error == nil {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if granted == true && error == nil {
if granted && error == nil {

.font(.title2.bold())
.multilineTextAlignment(.center)
.fixedSize(horizontal: false, vertical: true)
Text("Meshtastic")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When strings don't need to be translated, you can use String(verbatim:):

Suggested change
Text("Meshtastic")
Text(verbatim: "Meshtastic")

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants