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] set default starting tab #55

Merged
merged 4 commits into from Apr 25, 2020
Merged

Conversation

shreyas1599
Copy link
Contributor

Fixes #40

I used 4 StorageKeys, one for each platform. I cannot re-use the same key for all as the count of bottom bar items is not the same. Using the same key would've made this a whole lot easier.

Let me know if there are any changes to be made.

@pd4d10
Copy link
Owner

pd4d10 commented Apr 11, 2020

Thanks!

I'll take a look ASAP

@shreyas1599 shreyas1599 deleted the set-default-tab branch April 11, 2020 15:21
@shreyas1599 shreyas1599 restored the set-default-tab branch April 11, 2020 15:21
@shreyas1599
Copy link
Contributor Author

Sorry deleted the branch by mistake

@shreyas1599 shreyas1599 reopened this Apr 11, 2020
@shreyas1599
Copy link
Contributor Author

shreyas1599 commented Apr 14, 2020

@pd4d10 I figured out issues #57 and #54. They're tied to the same fix. I'll open a pr once you merge this as I have messed up my local repo and fork and both the issue fixes make changes in home.dart(which this pr modifies). I'll update them(local and fork) after this is merged and open a separate pr that fixes the above issues. Thanks.

@pd4d10
Copy link
Owner

pd4d10 commented Apr 17, 2020

After doing some research, I found that some apps would save the last active tab and use it at the next launch, for example, Apple App Store

Personally I think it seems better because users would not be bothered to do an extra setting step.

What do you think?

@shreyas1599
Copy link
Contributor Author

I see. That makes sense. So I'll modify this. But I still feel the setting to so do should be left as it is too. Or do you want to remove it?

@pd4d10
Copy link
Owner

pd4d10 commented Apr 17, 2020

the setting to so do should be left as it is too

It makes sense.

But it might be more complicated to implement this. We need to have two store keys for each platform, one is for the initiative settings (high priority), the other is for the tab item click (low priority). If the former is present the latter would have no effect.

It seems to be a lot of work, so I would recommend not including the settings code in this PR.

@shreyas1599
Copy link
Contributor Author

Alright. That makes sense. I'll change it.

@shreyas1599
Copy link
Contributor Author

shreyas1599 commented Apr 20, 2020

Apologies for taking so much time. There were a few problems that escaped my notice. Within the theme.dart I found many mistakes that somehow didn't affect setting a default start tab but affected this. I fixed those but there's still one problem for which I can't seem to find a fix. The GhNotificationScreen(when not the default tab), if tapped after the app start immediately switches to the previous saved defaultTabState. However, after this if it is clicked, there is no problem. The problem is with the screen as I used GhNewsScreen as a replacement and it seems to work fine.

Hopefully, the gif explains the problem. The problem is only with the Notification screen of github and does not affect other screens of other platforms.

https://gph.is/g/Z7WdrQP

Edit: The problem was that didChangeDependencies is called again for Notifications alone and not for other screens. I used a global flag to ensure it is called only once right after the widgets are built. Feels like a hacky fix. Let me know if you feel there's a better way to do it.

lib/models/theme.dart Outdated Show resolved Hide resolved
lib/home.dart Show resolved Hide resolved
@shreyas1599 shreyas1599 requested a review from pd4d10 April 25, 2020 12:08
lib/home.dart Show resolved Hide resolved
@pd4d10 pd4d10 merged commit 54a2d71 into pd4d10:master Apr 25, 2020
@shreyas1599 shreyas1599 deleted the set-default-tab branch May 8, 2020 17:57
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.

[Feature Request] Set a certain tab as home/starting tab
2 participants