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

Add Show Funding Menu settings to show/hide FundingMenu on PodcastDetails #32

Merged
merged 1 commit into from Mar 17, 2021

Conversation

erdemyerebasmaz
Copy link
Contributor

@erdemyerebasmaz erdemyerebasmaz commented Feb 24, 2021

Since we have our own payment system integrated into the plugin via hooks, this raised the need to hide FundingMenu on PodcastDetails page. We believe the least intrusive way to go with this change was to add a showFunding flag to AppSettings.

This PR introduces showFunding settings flag that allows developers to hide FundingMenu on PodcastDetails page.
This setting is true by default and won't be configurable through Settings page.

…ails

This setting will be true by default and won't be configurable through Settings.
@amugofjava
Copy link
Owner

Hi @erdemyerebasmaz,

Would a podcast not potentially support both Lightning and Funding links, or are you presenting the funding links within Breez itself?

@amugofjava amugofjava added the question Further information is requested label Mar 7, 2021
@kingonly
Copy link

kingonly commented Mar 7, 2021

@amugofjava only Lightning is supported in Breez

@amugofjava
Copy link
Owner

Hi, I've made a slight adjustment to podcast details. Returning a SizedBox whilst waiting for data to appear in the stream causes a noticeable screen jump as you can see in the first video below (slowed down to make it more visible). It shouldn't have any impact on the funding button, but let me know if you have any problems.

settings_jank.mp4
settings_fixed.mp4

@amugofjava amugofjava merged commit 4a88450 into amugofjava:master Mar 17, 2021
@erdemyerebasmaz
Copy link
Contributor Author

erdemyerebasmaz commented Mar 17, 2021

Hi, I've made a slight adjustment to podcast details. Returning a SizedBox whilst waiting for data to appear in the stream causes a noticeable screen jump as you can see in the first video below (slowed down to make it more visible). It shouldn't have any impact on the funding button, but let me know if you have any problems.

settings_jank.mp4
settings_fixed.mp4

Good catch! @amugofjava
I tested and unfortunately funding button is displayed till showFunding setting is retrieved. because stream is initialized with AppSettings.sensibleDefaults()

SVID_20210317_123623_1.mp4

What we should do is to move StreamBuilder inside Column children where FundingMenu is:

StreamBuilder<AppSettings>(
    stream: _settingsBloc.settings,
                ⋮
        child: [
        SubscriptionButton(podcast),
        PodcastContextMenu(podcast),
        settings.showFunding ? FundingMenu(podcast.funding) : Container(),
        ]
                ⋮
SubscriptionButton(podcast),
PodcastContextMenu(podcast),
StreamBuilder<AppSettings>(
   stream: _settingsBloc.settings,
   builder: (context, settingsSnapshot) {
      return (settingsSnapshot.hasData && settingsSnapshot.data.showFunding)
                 ? FundingMenu(podcast.funding)
                 : Container();
      },
),

This way the wait won't affect the whole widget.

@erdemyerebasmaz
Copy link
Contributor Author

@amugofjava Please see my comment above.

@amugofjava
Copy link
Owner

Hi @erdemyerebasmaz,

Yes, I'll give that a try and see if it improves things.

This was referenced Mar 17, 2021
@amugofjava
Copy link
Owner

Hi @erdemyerebasmaz,

I tried moving the stream closer to the widget using it. It's good practice but made no difference. So, I have changed the settings BLoC to allow direct access to the current settings. This does break the 'contract' that BLoCs should only be accessed via sinks and streams, but in this case I think it's more important to have a jank free ui! :) I hadn't noticed until I saw your video that the play button was janky too so I have also fixed that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants