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

fix: NavigationBarDestination.disabled has no visual effect #4073

Merged
merged 2 commits into from
Oct 17, 2024

Conversation

ndonkoHenri
Copy link
Contributor

@ndonkoHenri ndonkoHenri commented Oct 1, 2024

Description

Fixes #4029

Test Code

import flet as ft


def main(page: ft.Page):

    page.title = "NavigationBar Example"

    def handle_disable_change(e):
        page.navigation_bar.disabled = e.control.value
        page.update()

    def handle_adaptive_change(e):
        page.navigation_bar.adaptive = e.control.value
        page.update()

    page.navigation_bar = ft.NavigationBar(
        destinations=[
            ft.NavigationBarDestination(icon=ft.icons.EXPLORE, label="Explore"),
            ft.NavigationBarDestination(icon=ft.icons.COMMUTE, label="Commute"),
            ft.NavigationBarDestination(
                icon=ft.icons.BOOKMARK_BORDER,
                selected_icon=ft.icons.BOOKMARK,
                label="Explore",
            ),
        ],
    )
    page.add(
        ft.Switch("Disable", value=False, on_change=handle_disable_change),
        ft.Switch("Adaptive", value=False, on_change=handle_adaptive_change),
    )


ft.app(main)

Summary by Sourcery

Fix the visual effect of the disabled state in NavigationBarDestination and refactor the handling of adaptive and disabled states in navigation bar controls.

Bug Fixes:

  • Fix the issue where NavigationBarDestination.disabled had no visual effect by ensuring that the disabled state is properly handled in the navigation bar controls.

Enhancements:

  • Refactor the handling of adaptive and disabled states in navigation bar controls to improve code clarity and maintainability.

Copy link
Contributor

sourcery-ai bot commented Oct 1, 2024

Reviewer's Guide by Sourcery

This pull request addresses an issue where the NavigationBarDestination.disabled property had no visual effect. The changes implement proper handling of disabled states for both NavigationBar and CupertinoNavigationBar, improve code organization, and enhance the overall functionality of navigation components.

Sequence Diagram

sequenceDiagram
    participant User
    participant NavigationBar
    participant Destination
    User->>NavigationBar: Tap destination
    NavigationBar->>NavigationBar: Check disabled state
    alt NavigationBar is not disabled
        NavigationBar->>Destination: Check destination disabled state
        alt Destination is not disabled
            Destination->>NavigationBar: Allow selection
            NavigationBar->>User: Update selected index
        else Destination is disabled
            Destination->>NavigationBar: Prevent selection
        end
    else NavigationBar is disabled
        NavigationBar->>User: No action
    end
Loading

File-Level Changes

Change Details Files
Implement proper handling of disabled state for NavigationBar and CupertinoNavigationBar
  • Add disabled check to onDestinationSelected callback
  • Implement destinationDisabled flag for individual destinations
  • Update enabled property of NavigationDestination based on destinationDisabled
  • Modify onTap callback for CupertinoTabBar to respect disabled state
packages/flet/lib/src/controls/navigation_bar.dart
packages/flet/lib/src/controls/cupertino_navigation_bar.dart
Refactor and improve code organization
  • Add isAdaptive getter to Control class
  • Use parseDuration function for animationDuration
  • Simplify labelBehavior assignment
  • Remove redundant variable declarations
packages/flet/lib/src/controls/navigation_bar.dart
packages/flet/lib/src/models/control.dart
Enhance flexibility and consistency of navigation components
  • Add support for indicatorColor as a fallback for activeColor in CupertinoTabBar
  • Improve handling of adaptive property for individual destinations
  • Update tooltip handling to allow null values
packages/flet/lib/src/controls/navigation_bar.dart
packages/flet/lib/src/controls/cupertino_navigation_bar.dart

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it.

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey @ndonkoHenri - I've reviewed your changes and they look great!

Here's what I looked at during the review
  • 🟢 General issues: all looks good
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟢 Complexity: all looks good
  • 🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@FeodorFitsner FeodorFitsner merged commit 3fc6118 into main Oct 17, 2024
2 checks passed
@FeodorFitsner FeodorFitsner deleted the disabled-nav-bar branch October 17, 2024 17:53
pengwon added a commit to pengwon/flet that referenced this pull request Oct 30, 2024
* main: (31 commits)
  Migrate `colors` and `icons` variables to Enums (flet-dev#4180)
  feat: expose more properties in Controls (flet-dev#4105)
  feat!: Refactor `Badge` Control to a Dataclass; create new `Control.badge` property (flet-dev#4077)
  fix: clicking on `CupertinoContextMenuAction` doesn't close context menu (flet-dev#3948)
  fix dropdown `max_menu_height` (flet-dev#3974)
  Fix undefined name "Future" --> asyncio.Future (flet-dev#4230)
  wrap ListTile in material widget (flet-dev#4206)
  Update CONTRIBUTING.md (flet-dev#4208)
  TextField: suffix_icon, prefix_icon and icon can be Control or str (flet-dev#4173)
  feat!: enhance `Map` control (flet-dev#3994)
  skip running flutter doctor on windows if no_rich_output is True (flet-dev#4108)
  add --pyinstaller-build-args to pack cli command (flet-dev#4187)
  fix/feat: make `SearchBar`'s view height adjustable; add new properties (flet-dev#4039)
  fix: prevent button `style` from being modified in `before_update()` (flet-dev#4181)
  fix: disabling filled buttons is not visually respected (flet-dev#4090)
  when `label` is set, use `MainAxisSize.min` for the `Row` (flet-dev#3998)
  fix: `NavigationBarDestination.disabled` has no visual effect (flet-dev#4073)
  fix autofill in CupertinoTextField (flet-dev#4103)
  Linechart: jsonDecode tooltip before displaying (flet-dev#4069)
  fixed bgcolor, color and elevation (flet-dev#4126)
  ...
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.

NavigationBarDestination set disabled does nothing
2 participants