-
Notifications
You must be signed in to change notification settings - Fork 16
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
[iOS] UI bug on iPad when using filters in Discussion #333
[iOS] UI bug on iPad when using filters in Discussion #333
Conversation
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.
LGTM, just small code-styling changes
.foregroundColor(Theme.Colors.accentXColor) | ||
Text(viewModel.filterTitle.localizedValue) | ||
}) | ||
.confirmationDialog(DiscussionLocalization.Posts.Alert.makeSelection, |
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.
please follow style "each parameter on own line"
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.
done
titleVisibility: isPad ? .automatic : .visible, | ||
actions: { | ||
ForEach(viewModel.filterInfos) { info in | ||
Button(action: { |
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.
please follow style "each parameter on own line"
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.
done
.foregroundColor(Theme.Colors.accentXColor) | ||
Text(viewModel.sortTitle.localizedValue) | ||
}) | ||
.confirmationDialog(DiscussionLocalization.Posts.Alert.makeSelection, |
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.
please follow style "each parameter on own line"
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.
done
titleVisibility: isPad ? .automatic : .visible, | ||
actions: { | ||
ForEach(viewModel.sortInfos) { info in | ||
Button(action: { |
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.
please follow style "each parameter on own line"
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.
done
Button( | ||
action: { | ||
showFilterSheet = true | ||
}, |
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.
Here is extra white space and giving the warning Trailing Whitespace Violation: Lines should not have trailing whitespace (trailing_whitespace)
.
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.
done
Button( | ||
action: { | ||
viewModel.filter(by: info) | ||
}, |
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.
Same extra white space warning.
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.
done
Button( | ||
action: { | ||
showSortSheet = true | ||
}, |
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.
Same extra white space warning.
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.
done
Button( | ||
action: { | ||
viewModel.sort(by: info) | ||
}, |
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.
Same extra white space warning.
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.
done
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.
LGTM 👍
Related to: [iOS] UI bug on iPad when using filters in Discussion #308
Contains fix for iPad discussions filter and sort sheets