-
Notifications
You must be signed in to change notification settings - Fork 0
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
GT-2430 disable share links on cyoa #2256
GT-2430 disable share links on cyoa #2256
Conversation
…obal/godtools-swift into GT-2430-disable-share-links-on-cyoa # Conflicts: # godtools.xcodeproj/project.pbxproj
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feature/cyoa-tool-settings #2256 +/- ##
==============================================================
- Coverage 40.73% 40.72% -0.01%
==============================================================
Files 1116 1116
Lines 36492 36502 +10
==============================================================
+ Hits 14864 14865 +1
- Misses 21628 21637 +9 ☔ View full report in Codecov by Sentry. |
|
||
import Foundation | ||
|
||
protocol LinkShareable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @rachaelblue these changes are looking good. The View fix looks good too.
I'm wondering if we should think about adding the dependencies needed into the protocols? Going to think about that a bit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't me reviewing the PR haha, but @rachaelblue I really like how you set up Tool Options, allowing easy disable and enable of the different parts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @aaronlaib welcome. I agree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good @levieggertcru. And thanks @aaronlaib!
@@ -28,15 +28,18 @@ struct ToolSettingsOptionsView: View { | |||
ScrollView(.horizontal, showsIndicators: false) { | |||
HStack { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might make a separate ticket to add a ForEach here and do that later. I should have originally done that. We could then just check for toolOptions.isEmpty in ToolSettingsView which would be a little safer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @rachaelblue this looks good. I ended up making a separate ticket (https://jira.cru.org/browse/GT-2434) for adding a ForEach to the HStack for tool options. I should've done that originally. Not a high priority, but something that would be nice to update to in the future.
No description provided.