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

feat: Add a builder for speakerInfo #68

Closed

Conversation

chooyan-eng
Copy link
Contributor

@chooyan-eng chooyan-eng commented Mar 5, 2024

Status

READY

Description

Related to #62, I found another issue that speakerInfo can't be localized as the object is created outside of FlutterDeckApp, meaning outside of MaterialApp.

I could solve the problem by wrapping FlutterDeckApp with another MaterialApp like below

MaterialApp(
  debugShowCheckedModeBanner: false,
  localizationsDelegates: AppLocalizations.localizationsDelegates,
  supportedLocales: AppLocalizations.supportedLocales,
  home: Builder(
    builder: (c) => FlutterDeckApp(
      speakerInfo: FlutterDeckSpeakerInfo(
        name: AppLocalizations.of(c)!.name,
        description: AppLocalizations.of(c)!.occupation,
        imagePath: 'assets/images/${AppLocalizations.of(c)!.imageFile}',
        socialHandle: AppLocalizations.of(c)!.social,
      ),
    ),
  ),
);

but that seems inefficient, and the biggest problem is that changing the locale feature of the control is not applied on FlutterDeckTitleSlide.

Here is my proposal to add a Builder version of speakerInfo which allows users to use BuildContext that is at least a descendant of FlutterDeckApp.

This enables users to simply write like below.

FlutterDeckApp(
  speakerInfoBuilder: (context) => FlutterDeckSpeakerInfo(
    name: AppLocalizations.of(context)!.name,
    description: AppLocalizations.of(context)!.occupation,
    imagePath: 'assets/images/${AppLocalizations.of(context)!.imageFile}',
    socialHandle: AppLocalizations.of(context)!.social,
  ),
);

Type of Change

  • ✨ New feature (non-breaking change which adds functionality)
  • πŸ› οΈ Bug fix (non-breaking change which fixes an issue)
  • ❌ Breaking change (fix or feature that would cause existing functionality to change)
  • 🧹 Code refactor
  • βœ… Build configuration change
  • πŸ“ Documentation
  • πŸ—‘οΈ Chore

Copy link
Owner

@mkobuolys mkobuolys left a comment

Choose a reason for hiding this comment

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

Hey @chooyan-eng, thanks for your PR. Honestly, after introducing localization to slides, I am not happy with several things that were implemented without considering l10n. E.g. slide header/footer cannot be translated as well as they are currently provided via the FlutterDeckSlideConfiguration that has no access to context.

Having this in mind, can we put the speakerInfoBuilder as a property to the FlutterDeckSlide.title template instead of providing it globally? For the global approach, I will revisit all the FlutterDeckSlideConfiguration values that could be affected by the localization change separately.

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.

None yet

2 participants