Skip to content
This repository has been archived by the owner on Jan 5, 2023. It is now read-only.

Bottom nav bar should depend on flag #2

Merged
merged 2 commits into from
Oct 18, 2020

Conversation

zzmren
Copy link

@zzmren zzmren commented Jun 3, 2020

Fixing an issue where settings showNavigation to false didn't do anything.

Similar bug on forked repo CrossPT#57

@@ -225,7 +225,7 @@ class _PDFViewerState extends State<PDFViewer> {
)
: null,
floatingActionButtonLocation: FloatingActionButtonLocation.centerDocked,
bottomNavigationBar: (widget.showNavigation || widget.document.count > 1)
bottomNavigationBar: widget.showNavigation

Choose a reason for hiding this comment

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

Instead of removing the 2nd part of the condition, we can just change OR to AND.

Choose a reason for hiding this comment

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

Can you please take a look at this PR @lohanidamodar

Copy link
Owner

Choose a reason for hiding this comment

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

@Monal5031 I believe your suggestion is better, as we don't want the navigation shown even if the showNavigation is true and page count is just 1. AND instead of removing second condition seems better.

@dimitri-bret
Copy link

this would be great ;) I struggled to find the problem untill i saw the condition in the code, and this issue.

@Monal5031
Copy link

@dimitri-bret There is a workaround you can use, until this is merged. pass navigationBuilder: null in the widget call.

Copy link
Owner

@lohanidamodar lohanidamodar left a comment

Choose a reason for hiding this comment

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

Can you please use && instead of removing second condition? That's a better solution as mentioned by @Monal5031 .
Thank you for your PR and support

@zzmren
Copy link
Author

zzmren commented Oct 18, 2020

Added the requested change @lohanidamodar

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants