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

[Bug] SettingPicker icon not rendered in combination with ChoicesConfiguration.pickerDisplayMode(.menu) #44

Open
duraki opened this issue Jan 26, 2025 · 2 comments

Comments

@duraki
Copy link

duraki commented Jan 26, 2025

While experimenting with PR #19 by @Saik0s, I've found a bug which results in SettingPicker component not rendering the icon, as it should. This happens when used in combination with ChoicesConfiguration.pickerDisplayMode(.menu) instead of revolving around default .navigation display mode, which works.

Bug Reproduction

To reproduce this bug and make icon not showing use the following code (notably choiceConfiguration: (...) constructor param:

// ...
struct SomeView: View {
  @AppStorage("pickerIndex") var pickerIndex = 0

  var body: some View {
    // ...
    SettingGroup(header: "Text Appearance", footer: "Use this settings to customize text rendering styles.") {
      SettingPicker(
        icon: .system(icon: "textformat.subscript", backgroundColor: .pink),
        title: "Example Picker",
        choices: ["Default", "Gradient", "Image based"],
        selectedIndex: $pickerIndex,
        choicesConfiguration: .init(pickerDisplayMode: .menu)  /** bug produced by this line */
      )
    }
  }
}

Example of working code when the icon is shown, using .navigation display mode instead:

// ...
SettingGroup(header: "...", footer: "...") {
    SettingPicker(
        // ...
        choicesConfiguration: .init(pickerDisplayMode: .navigation)
    )
}

Rendered Results

Left side is working example when using .navigation display mode, while the right side is showing bugged version. The important item is the "Vehicle Model" settings (last item).

Expected results due to .navigation disp. mode         Unexpected result due to .menu disp. mode

@duraki
Copy link
Author

duraki commented Jan 26, 2025

Code used while debugging in Example/SettingExample XCode previews:

struct SettingPicker_Previews: PreviewProvider {
    static var previews: some View {
        PreviewWrapper()
    }

    struct PreviewWrapper: View {
        @State private var selectedIndex = 0

        var body: some View {
            VStack {
                SettingPicker(
                    icon: .system(icon: "chevron.compact.down", backgroundColor: .black),
                    title: "SettingPicker ~ Navigation",
                    choices: ["Option 1", "Option 2"],
                    selectedIndex: $selectedIndex,
                    choicesConfiguration: SettingPicker
                        .ChoicesConfiguration(
                            pickerDisplayMode: .navigation
                        )
                )

                SettingPicker(
                    icon: .system(icon: "chevron.compact.up", backgroundColor: .black),
                    title: "SettingPicker ~ Menu",
                    choices: ["Option 1", "Option 2"],
                    selectedIndex: $selectedIndex,
                    choicesConfiguration: SettingPicker
                        .ChoicesConfiguration(
                            pickerDisplayMode: .menu
                        )
                )
            }
        }
    }
}

duraki added a commit to When-original-devs-wont-merge-good-PRs/Setting that referenced this issue Jan 26, 2025
…menu` _ref aheze#44

This small PR fixes bug in aheze#44 where the icon is not showing in SettingPicker component, when used in combination with `Setting.ChoicesConfiguration(pickerDisplayMode: .menu, ....)`:

- Preappends `SettingIconView(...)` when using `.menu` picker's display mode
- Requires merge of base PR @ aheze#43 & aheze#42 or cherry-picking
@duraki
Copy link
Author

duraki commented Jan 26, 2025

Here is the patch diff that fixes this When-original-devs-wont-merge-good-PRs@fac1bd9:

index f255d41..fd90f83 100644
--- a/Sources/Views/SettingPicker.swift
+++ b/Sources/Views/SettingPicker.swift
@@ -182,6 +182,10 @@ struct SettingPickerView: View {
 
         case .menu:
             HStack(spacing: horizontalSpacing) {
+                if let icon {
+                    SettingIconView(icon: icon)
+                }
+
                 Text(title)
                     .fixedSize(horizontal: false, vertical: true)
                     .frame(maxWidth: .infinity, alignment: .leading)

Usage: git apply SettingPicker_Icon_MenuDispMode.patch

PR not yet created due to merge requirements of #43. Otherwise, cherry-picking and commit by @aheze is also appreciated. The implementation of .inline display mode doesn't make much sense since it's not showing picker's descriptive text/label, therefore the icons would render on all available choices.

Patch above fixes

Image

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

No branches or pull requests

1 participant