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: extract subviews on extension #186

Merged
merged 8 commits into from May 13, 2024

Conversation

Harry-KNIGHT
Copy link
Contributor

I found that settings view was really big, so I decided to extract them on method returning some View for a better readability.

If you have any feedback do not hesitate to reach me or comment the PR.

I think we can have a better naming for methods but I am not much inspired now

@twostraws
Copy link
Owner

This is a great idea! I'd be curious why these can't be pulled out to separate views entirely, rather than leaving them as methods.

@Harry-KNIGHT
Copy link
Contributor Author

This is a great idea! I'd be curious why these can't be pulled out to separate views entirely, rather than leaving them as methods.

I have used methods that return views because what they do is quite explicit. This limits the number of files and previews to be added in addition to putting fewer lines to the file.

If it's better for the project, I can use structs to keep the homogeneity of the app

@twostraws
Copy link
Owner

Once we move over to @Observable (when macOS 15 is likely to be released around October), it's more efficient to create smaller views than one big one, even when methods are involved. So, let's plan ahead, just like we're moving towards Swift 6 already 🙂

@Harry-KNIGHT
Copy link
Contributor Author

Harry-KNIGHT commented May 6, 2024

Once we move over to @Observable (when macOS 15 is likely to be released around October), it's more efficient to create smaller views than one big one, even when methods are involved. So, let's plan ahead, just like we're moving towards Swift 6 already 🙂

Yeah for sure we can take get ahead, the problem with my extension of the view is the coupling of subview with the main view, I think it's better to use separate view entirely.

I'll make the changes soon 🔜

@Harry-KNIGHT
Copy link
Contributor Author

This works as expected, if you see some improvements do not hesitate to told me @twostraws

Screen.Recording.2024-05-11.at.09.02.37.mov

@twostraws
Copy link
Owner

This is perfect – thank you!

@twostraws twostraws merged commit d35ca96 into twostraws:main May 13, 2024
1 check passed
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