-
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
chore: Make inactive posting buttons/fields for "posting disabled" courses #377
Conversation
# Conflicts: # Discussion/Discussion/Presentation/Comments/Responses/ResponsesView.swift # Discussion/Discussion/Presentation/Comments/Thread/ThreadView.swift # Discussion/Discussion/Presentation/DiscussionTopics/DiscussionTopicsView.swift # Discussion/Discussion/Presentation/DiscussionTopics/DiscussionTopicsViewModel.swift # Discussion/Discussion/Presentation/Posts/PostsView.swift
I think we need to specifically disable these options: Post/Response/Comment, but not hide. @saeedbashir what do you think? |
@rnr I've discussed it with @moiz994 and he was okay with hiding the buttons. The only concern was about the banner( |
let start = Date(iso8601: blackout.start) | ||
let end = Date(iso8601: blackout.end) |
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.
there is an extra space after :
@@ -249,7 +253,8 @@ struct ResponsesView_Previews: PreviewProvider { | |||
commentID: "", | |||
viewModel: viewModel, | |||
router: router, | |||
parentComment: post | |||
parentComment: post, |
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.
extra white space st the end
@@ -259,7 +264,8 @@ struct ResponsesView_Previews: PreviewProvider { | |||
commentID: "", | |||
viewModel: viewModel, | |||
router: router, | |||
parentComment: post | |||
parentComment: post, |
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.
private let threadStateSubject: CurrentValueSubject<ThreadPostState?, Never> | ||
|
||
|
||
public var isBlackedOut: Bool = false |
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.
Do we need to have it in viewModel? isn't it good to declare a variable in the view and use it directly
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 think better use only state variable in view, others variable in view model, and view model say how view should look
func generateTopics(topics: Topics?) -> [DiscussionTopic] { | ||
var result = [ | ||
DiscussionTopic( | ||
name: DiscussionLocalization.Topics.allPosts, | ||
action: { | ||
self.analytics.discussionAllPostsClicked(courseId: self.courseID, | ||
courseName: self.title) | ||
self.analytics.discussionAllPostsClicked( |
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.
As we are here how about using weak self in block?
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.
We don't need weak self for button action from view
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 👍
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.
👍
In some courses (with "Archived" status) posting in Discussions is disabled ("Posting in discussions is disabled by the course team"). On the Web and in edX app all make Post/Response/Comment options are hidden or inactive. We also need to disable (or hide) the "Create new post" button and the "Add a response" / "Add a comment" fields for these courses.
Make inactive posting buttons/fields for "posting disabled" courses #350
Increase tappable area size for sequence elements in Coursework #353