From jb55@jb55.com Fri Jul 14 17:53:07 2023 Return-Path: Delivered-To: jb55 Received: from localhost ([::1]) by charon.jb55.com with LMTP id Jl/FOYaLsWSDJhAABqXyzQ (envelope-from ) for ; Fri, 14 Jul 2023 10:53:10 -0700 Return-Path: Delivered-To: jb55@jb55.com Received: from jb55.com (S010660e327dca171.vc.shawcable.net [24.84.152.187]) by jb55.com (OpenSMTPD) with ESMTPSA id 64346ac2 (TLSv1.3:AEAD-AES256-GCM-SHA384:256:NO); Fri, 14 Jul 2023 17:53:10 +0000 (UTC) Date: Fri, 14 Jul 2023 10:53:07 -0700 From: William Casarin To: Suhail Saqan Cc: patches@damus.io Subject: Re: [PATCH 1/8] add ReactionsSettingsView page and settings variables Message-ID: References: <20230713212605.81379-1-suhail.saqan@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20230713212605.81379-1-suhail.saqan@gmail.com> Hey Suhail, Comments inline: On Thu, Jul 13, 2023 at 04:25:58PM -0500, Suhail Saqan wrote: >--- > damus/Models/UserSettingsStore.swift | 6 + > damus/Util/Router.swift | 7 + > damus/Views/ConfigView.swift | 4 + > .../Settings/ReactionsSettingsView.swift | 267 ++++++++++++++++++ > [..] > @Setting(key: "donation_percent", default_value: 0) > var donation_percent: Int >+ >+ @Setting(key: "emoji_reactions", default_value: default_emoji_reactions) >+ var emoji_reactions: [String] I believe you need UserDefaults.settings.stringArray for this setting to work. I don't think the current @Setting property wrapper supports this? >diff --git a/damus/Views/Settings/ReactionsSettingsView.swift b/damus/Views/Settings/ReactionsSettingsView.swift >new file mode 100644 >index 00000000..cfe98f64 >--- /dev/null >+++ b/damus/Views/Settings/ReactionsSettingsView.swift > [..] >+ } header: { >+ HStack { >+ Text(NSLocalizedString("Add Emoji", comment: "Label for section for adding an emoji to the reactions list.")) >+ .font(.system(size: 18, weight: .heavy)) >+ .padding(.bottom, 5) There is only one element in this HStack, can just rmove? >+ } >+ } footer: { >+ VStack { This VStack seems to only have 1 element? We can just remove it. >+ HStack { >+ Spacer() >+ if(!new_emoji.isEmpty) { nit: if !new_emoji.isEmpty { >+ Button(NSLocalizedString("Cancel", comment: "Button to cancel out of view adding user inputted emoji.")) { >+ new_emoji = "" >+ } >+ .font(.system(size: 14, weight: .bold)) >+ .frame(width: 80, height: 30) > [..] >+ .clipShape(Capsule()) >+ .padding(EdgeInsets(top: 15, leading: 0, bottom: 0, trailing: 0)) >+ >+ Button(NSLocalizedString("Add", comment: "Button to confirm adding user inputted emoji.")) { >+ if (isValidEmoji(new_emoji)) { nit: if isValidEmoji(new_emoji) { >+ settings.emoji_reactions.append(new_emoji) >+ new_emoji = "" >+ } >+ } >+ .font(.system(size: 14, weight: .bold)) > [..] >+ } >+ } >+ >+ Section { >+ List(Array(settings.emoji_reactions), id: \.self) { emoji in isn't this already an Array? >+ EmojiView(settings: settings, emoji: emoji, recommended: false, showActionButtons: $showActionButtons) >+ } >+ } header: { >+ HStack { >+ Text(NSLocalizedString("Emoji Reactions", comment: "Section title for emoji reactions that are currently added.")) Text's are already localizable, you can just do: Text("Emoji Reactions", comment: ...) >+ .font(.system(size: 18, weight: .heavy)) >+ .padding(.bottom, 5) >+ } >+ } >+ > [..] >+ List(Array(recommended), id: \.self) { emoji in >+ EmojiView(settings: settings, emoji: emoji, recommended: true, showActionButtons: $showActionButtons) >+ } >+ } header: { >+ Text(NSLocalizedString("Recommended Emojis", comment: "Section title for recommend emojis")) Text("Recommended Emojis", comment: ...) >+ .font(.system(size: 18, weight: .heavy)) >+ .padding(.bottom, 5) >+ } >+ } >+ } > [..] >+ } >+ } >+ } >+ >+ func getMissingRecommendedEmojis(added: [String], recommended: [String] = default_emoji_reactions) -> [String] { needs a code comment, I don't know what this function is for >+ let addedSet = Set(added) >+ let missingEmojis = recommended.filter { !addedSet.contains($0) } >+ return missingEmojis >+ } >+} > [..] >+ } >+ } >+} >+ >+struct EmojiView: View { there are too many views in this file, let's make sure views like this are in their own file and have their own previews so that we can use the preview feature in xcode. >+ @ObservedObject var settings: UserSettingsStore >+ >+ let emoji: String >+ let recommended: Bool >+ > [..] >+ Text(emoji) >+ } >+ } >+ .swipeActions { >+ if (!recommended){ nit: if !recommended { >+ RemoveButton() >+ .tint(.red) >+ } else { >+ AddButton() >+ .tint(.green) > [..] >+ func RemoveButton() -> some View { >+ Button(action: { >+ if let index = settings.emoji_reactions.firstIndex(of: emoji) { >+ settings.emoji_reactions.remove(at: index) >+ } would we ever not have an index here? >+ }) { >+ Image(systemName: "minus.circle") >+ .resizable() >+ .frame(width: 20, height: 20) >+ .foregroundColor(.red) > [..] >+ } >+} >+ >+/// From: https://stackoverflow.com/a/39425959 >+extension Character { Let's put this in its own file >+ /// A simple emoji is one scalar and presented to the user as an Emoji >+ var isSimpleEmoji: Bool { >+ guard let firstScalar = unicodeScalars.first else { return false } >+ return firstScalar.properties.isEmoji && firstScalar.value > 0x238C >+ } > [..] >+struct ReactionsSettingsView_Previews: PreviewProvider { >+ static var previews: some View { >+ ReactionsSettingsView(settings: UserSettingsStore()) >+ } >+} Getting there, thank you! jb