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

SideBarItem leading icon theme should respect configured theme #313

Closed
keaganhilliard opened this issue Oct 23, 2022 · 8 comments · Fixed by #374
Closed

SideBarItem leading icon theme should respect configured theme #313

keaganhilliard opened this issue Oct 23, 2022 · 8 comments · Fixed by #374
Assignees
Labels
bug Something isn't working

Comments

@keaganhilliard
Copy link

keaganhilliard commented Oct 23, 2022

Description

The SideBarItem leading widget is hardcoded to blue.

if (hasLeading)
  Padding(
    padding: EdgeInsets.only(right: spacing),
    child: MacosIconTheme.merge(
      data: MacosIconThemeData(
        color: selected
            ? MacosColors.white
            : MacosColors.controlAccentColor,
        size: itemSize.iconSize,
      ),
      child: item.leading!,
    ),
  ),

Steps To Reproduce

  1. Change the MacosIconTheme color to anything other than the MacOS blue.
  2. Create a sidebar
  3. Add sidebar items with MacosIcons in the leading widget
  4. Notice that the defined theme icon color is not respected.
Code sample
import 'package:flutter/cupertino.dart';
import 'package:flutter/material.dart';
import 'package:macos_ui/macos_ui.dart';

void main() {
  runApp(const App());
}

class App extends StatelessWidget {
  const App({super.key});

  // This widget is the root of your application.
  @override
  Widget build(BuildContext context) {
    return MacosApp(
      title: 'Audiobookly',
      theme: MacosThemeData.light().copyWith(
        iconTheme: const MacosIconThemeData(
          color: Colors.deepPurple,
        ),
      ),
      darkTheme: MacosThemeData.dark().copyWith(
        primaryColor: Colors.deepPurple,
        pushButtonTheme:
            const PushButtonThemeData(color: Color.fromRGBO(103, 58, 183, 1)),
        iconTheme: const MacosIconThemeData(
          color: Colors.deepPurple,
        ),
      ),
      themeMode: ThemeMode.system,
      home: const MainView(),
      debugShowCheckedModeBanner: false,
    );
  }
}

class MainView extends StatefulWidget {
  const MainView({super.key});

  @override
  State<MainView> createState() => _MainViewState();
}

class _MainViewState extends State<MainView> {
  int _pageIndex = 0;

  @override
  Widget build(BuildContext context) {
    var color = MacosTheme.of(context).iconTheme.color;
    return PlatformMenuBar(
      menus: const [
        PlatformMenu(
          label: 'Bug',
          menus: [
            PlatformProvidedMenuItem(
              type: PlatformProvidedMenuItemType.about,
            ),
            PlatformProvidedMenuItem(
              type: PlatformProvidedMenuItemType.quit,
            ),
          ],
        ),
      ],
      child: MacosWindow(
        sidebar: Sidebar(
          minWidth: 200,
          builder: (context, scrollController) => SidebarItems(
            currentIndex: _pageIndex,
            onChanged: (index) {
              setState(() => _pageIndex = index);
            },
            items: const [
              SidebarItem(
                leading: MacosIcon(CupertinoIcons.home),
                label: Text('Home'),
              ),
              SidebarItem(
                leading: MacosIcon(CupertinoIcons.home),
                label: Text('Home'),
              ),
            ],
          ),
        ),
        child: IndexedStack(
          index: _pageIndex,
          children: const [
            HomePage(),
          ],
        ),
      ),
    );
  }
}

class HomePage extends StatelessWidget {
  const HomePage({super.key});

  @override
  Widget build(BuildContext context) {
    return Builder(
      builder: (context) {
        return MacosScaffold(
          toolBar: ToolBar(
            title: const Text('Home'),
            actions: [
              ToolBarIconButton(
                label: 'Toggle Sidebar',
                icon: const MacosIcon(CupertinoIcons.sidebar_left),
                showLabel: false,
                tooltipMessage: 'Toggle Sidebar',
                onPressed: () {
                  MacosWindowScope.of(context).toggleSidebar();
                },
              )
            ],
          ),
          children: [
            ContentArea(
              builder: (context, scrollController) {
                return const Center(
                  child: Text('Home'),
                );
              },
            ),
          ],
        );
      },
    );
  }
}

Expected behavior

I would expect the Leading widget to respect the configured theme.

Screenshots

image

Logs

Logs
N/A
N/A
[✓] Flutter (Channel master, 3.4.0-38.0.pre.2, on macOS 12.6 21G115
    darwin-arm64, locale en-US)
    • Flutter version 3.4.0-38.0.pre.2 on channel master at
      /Users/keagan/Code/flutter
    • Upstream repository https://github.com/flutter/flutter.git
    • Framework revision 3028db7178 (3 weeks ago), 2022-10-04 00:11:24 -0400
    • Engine revision b9e15923bf
    • Dart version 2.19.0 (build 2.19.0-268.0.dev)
    • DevTools version 2.17.0

[✓] Android toolchain - develop for Android devices (Android SDK version
    33.0.0)
    • Android SDK at /Users/keagan/Library/Android/sdk
    • Platform android-33, build-tools 33.0.0
    • Java binary at: /Users/keagan/Library/Application
      Support/JetBrains/Toolbox/apps/AndroidStudio/ch-0/213.7172.25.2113.9014
      738/Android Studio.app/Contents/jre/Contents/Home/bin/java
    • Java version OpenJDK Runtime Environment (build
      11.0.13+0-b1751.21-8125866)
    • All Android licenses accepted.

[✓] Xcode - develop for iOS and macOS (Xcode 14.0.1)
    • Xcode at /Applications/Xcode.app/Contents/Developer
    • Build 14A400
    • CocoaPods version 1.11.3

[✓] Chrome - develop for the web
    • Chrome at /Applications/Google Chrome.app/Contents/MacOS/Google Chrome

[✓] Android Studio (version 2021.2)
    • Android Studio at /Applications/Android Studio.app/Contents
    • Flutter plugin can be installed from:
      🔨 https://plugins.jetbrains.com/plugin/9212-flutter
    • Dart plugin can be installed from:
      🔨 https://plugins.jetbrains.com/plugin/6351-dart
    • Java version OpenJDK Runtime Environment (build
      11.0.12+0-b1504.28-7817840)

[✓] Android Studio (version 2021.2)
    • Android Studio at /Users/keagan/Library/Application
      Support/JetBrains/Toolbox/apps/AndroidStudio/ch-0/212.5712.43.2112.8815
      526/Android Studio.app/Contents
    • Flutter plugin can be installed from:
      🔨 https://plugins.jetbrains.com/plugin/9212-flutter
    • Dart plugin can be installed from:
      🔨 https://plugins.jetbrains.com/plugin/6351-dart
    • Java version OpenJDK Runtime Environment (build
      11.0.12+0-b1504.28-7817840)

[✓] Android Studio (version 2021.3)
    • Android Studio at /Users/keagan/Library/Application
      Support/JetBrains/Toolbox/apps/AndroidStudio/ch-0/213.7172.25.2113.9014
      738/Android Studio.app/Contents
    • Flutter plugin can be installed from:
      🔨 https://plugins.jetbrains.com/plugin/9212-flutter
    • Dart plugin can be installed from:
      🔨 https://plugins.jetbrains.com/plugin/6351-dart
    • Java version OpenJDK Runtime Environment (build
      11.0.13+0-b1751.21-8125866)

[✓] VS Code (version 1.72.1)
    • VS Code at /Applications/Visual Studio Code.app/Contents
    • Flutter extension version 3.50.0

[✓] Connected device (2 available)
    • macOS (desktop) • macos  • darwin-arm64   • macOS 12.6 21G115
      darwin-arm64
    • Chrome (web)    • chrome • web-javascript • Google Chrome
      106.0.5249.119

[✓] HTTP Host Availability
    • All required HTTP hosts are available

• No issues found!
@keaganhilliard keaganhilliard added the bug Something isn't working label Oct 23, 2022
@nu11ptr
Copy link
Contributor

nu11ptr commented Oct 24, 2022

Related to #243 I think?

@keaganhilliard
Copy link
Author

Related in that the app is not respecting the OS control accent color for sure, but it's also not overridable when it should be.

@GroovinChip
Copy link
Collaborator

Hi @keaganhilliard, thanks for filing this issue.

I'm a little confused - are you suggesting that the icons in your sidebar should all be purple? Or that the currently selected item's icon should be purple?

@keaganhilliard
Copy link
Author

I would expect the icon to be purple when not active. The active one works fine.

@GroovinChip
Copy link
Collaborator

@keaganhilliard OK, I've reproduced the bug. Will look into the cause.

@GroovinChip GroovinChip self-assigned this Feb 16, 2023
@GroovinChip
Copy link
Collaborator

GroovinChip commented Feb 16, 2023

Ha, that was easy - the cause is that the blue color was hardcoded in. I'll get the fix up shortly.

EDIT: Actually, I suspect the problem is due to the MacosIconTheme.merge not properly retrieving the existing icon theme. So this needs further inspection.

EDIT 2: The correct color values do actually come through. Investigating further.

@GroovinChip
Copy link
Collaborator

Got it! The will go live soon

GroovinChip added a commit that referenced this issue Feb 28, 2023
#374)

* fix(SidebarItem): use theme's primary color instead of hardcoded value

Fixes #313

* chore: update version, changelog
GroovinChip added a commit that referenced this issue Feb 28, 2023
* feat: Sliver toolbar (#368)

* feat: `SliverToolBar`

* chore(example): address lints

* docs(SliverToolbar): update `floating` doc

* feat(example): add page for SliverToolbar

* chore: dart format

* docs(SliverToolBar): slight change to `pinned` docs

* docs(SliverToolBar): add section to readme

* docs(SliverToolBar): tweak sample

* test(SliverAppBar): add initial tests

* chore: update version & changelog

* chore: pub upgrade

* chore: remove unused imports

* test: remove ignore lint

* fix(SidebarItem): use theme's primary color instead of hardcoded value  (#374)

* fix(SidebarItem): use theme's primary color instead of hardcoded value

Fixes #313

* chore: update version, changelog
@keaganhilliard
Copy link
Author

Sweet, thank you!

GroovinChip added a commit that referenced this issue Apr 6, 2023
#374)

* fix(SidebarItem): use theme's primary color instead of hardcoded value

Fixes #313

* chore: update version, changelog
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants