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

feature: Use NavigationSplitView rather than TabView for iPad #6 #279

Conversation

windrunner21
Copy link
Contributor

What it Does

  • Closes FEATURE - Use NavigationSplitView rather than TabView for iPad #6
  • I have added NavigationSplitView logic into MainTabView struct. This addition is mutating only iPad version right now (completely works) and should affect MacOS version as well in the future (MacOS isn't building as of now). iPhone proceeds with TabView navigation. I have also refactored the code - added extension to hold @ViewBuilder views for better reusability and more readable code - because at the end of the day TabView and NavigationSplitView use the same content.

How I Tested

  • I have tested this new feature on Previews first and later on Simulators.
  • Tested on iPhone 15 Pro Max - 17.0.1
  • Tested on iPad Pro 12.9 inch - 17.0.1
  • iPad shows NavigationSplitView navigation - can be toggled to be shown/hidden.
  • iPhone shows TabView navigation.

Notes

  • There is a comment I have left in the code that TabSelection enum can be changed to conform to String rather than Int. If needed can be easily swiped out, currently is left as is.

Screenshot

  • Two screen recordings - iPad and iPhone.
Simulator.Screen.Recording.-.iPad.Pro.12.9-inch.6th.generation.-.2023-12-18.at.11.55.01.mp4
Simulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2023-12-18.at.11.55.50.mp4

@windrunner21
Copy link
Contributor Author

@mikaelacaron hi, hope all is well 😄 did you have the chance to look at the PR?

@mikaelacaron
Copy link
Owner

@windrunner21 No I haven't, and it's the holidays so I'm not going to be looking at this today. You'll see my review when I have time

@windrunner21
Copy link
Contributor Author

@mikaelacaron oh sorry, you are right, I totally forgot about it. Merry Christmas! Have a great holiday 🥳

Copy link
Owner

@mikaelacaron mikaelacaron left a comment

Choose a reason for hiding this comment

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

Great work!! I have several suggestions and a few things to fix.

Also can you order the functions navigationSplitView, tabView, and then selectionContent

There's also a bug, the tab view is saved when you close the app and open it again (for iPhone), it remembers which tab you were on, I'd like that same functionality for the navigation split view

@windrunner21
Copy link
Contributor Author

hey @mikaelacaron, sorry for the late reply, still on holidays! I'll update PR with requested changes by Sunday latest.

@mikaelacaron
Copy link
Owner

@windrunner21 No worries at all! Take vacation! 👍

@mikaelacaron
Copy link
Owner

@windrunner21 After you address all the comments, please click the re-review button

Re-review button

Also if you fix something I mentioned and don't have any questions, go ahead and click the Resolve button, but if you have a question, you can comment on that question directly

@windrunner21
Copy link
Contributor Author

@mikaelacaron I didn't click it yet because I still have to solve the bug with reopening the same tab after relaunching application. I wanted to solve everything and then request review 😄

@mikaelacaron
Copy link
Owner

Yup! You should resolve everything for sure! I just mentioned this cause I got notified you were working on it haha

And I saw that I didn't mention it in my first review

@windrunner21
Copy link
Contributor Author

@mikaelacaron just finished with the bug fix, implemented it in such a way that TabView / NavigationSplitView are having the same last opened tab.

@mikaelacaron mikaelacaron merged commit cb560d6 into mikaelacaron:dev Jul 8, 2024
2 checks passed
@mikaelacaron
Copy link
Owner

@windrunner21 thanks for working on this! haha just now finally getting around to finishing up the final bits of this project to launch it

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 - Use NavigationSplitView rather than TabView for iPad
2 participants