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

Forum Thread Status #2341

Merged
merged 15 commits into from Jul 17, 2018

Conversation

Projects
None yet
4 participants
@scopeInfinity
Copy link
Member

scopeInfinity commented Jul 4, 2018

Closes #2323
For now, PR includes three thread status

  • Comment
  • Unresolved (Default Option)
  • Resolved

Thread Status is initialized from Create Thread page, and updated from Edit Thread form.
Thread Status can be used for filter thread list in view thread page.

To be modified after #2301 : Allow the original author to edit thread status

@scopeInfinity scopeInfinity requested a review from andrewaikens87 Jul 4, 2018

@ropern123 ropern123 self-requested a review Jul 6, 2018

@andrewaikens87

This comment has been minimized.

Copy link
Member

andrewaikens87 commented Jul 7, 2018

Some things I noticed after taking a look:

  • As user student who created a thread I am unable to change the status of that thread. Seems like this privilege is only available to rank >= 2 (essentially those who can edit posts currently, which you have an open PR for so we might need to merge this after that one)
  • I don't think this is related but I noticed that if you open up the edit popup and then close it without modifying anything and try to navigate to a different page it alerts the user that changes that were made may not be saved, but no changes were made.
  • When editing a post that isn't the thread post (meaning one that is a reply), the dropdown is displayed for selecting whether the thread is resolved or not. This should only be available on the thread post.
  • When creating a thread there are three options: Default, Unresolved, Resolved as a user I feel like this is might be sort of confusing. Does it make sense to be creating a thread that starts off resolved? Isn't that more of a comment, which could be put under the Default category? This is probably something that needs to be discussed.
@andrewaikens87
Copy link
Member

andrewaikens87 left a comment

Commented above

@ropern123
Copy link
Contributor

ropern123 left a comment

Andrew got pretty much all of it.
Only really minor thing I noticed was that the icons on the threads (the resolved check and the unresolved clock(?)) are somewhat uncomfortably close to the pin and/or star when they are there.

@scopeInfinity

This comment has been minimized.

Copy link
Member Author

scopeInfinity commented Jul 11, 2018

Thankyou @andrewaikens87 @ropern123
PR is updated accordingly.

And bug(2) related to are-you-sure on popup form is already fixed in #2301
Also for allowing the student to edit their own thread status, I'll update this PR after #2301.

scopeInfinity added some commits Jul 14, 2018

@bmcutler

This comment has been minimized.

Copy link
Contributor

bmcutler commented Jul 16, 2018

Hi @gagan, can you please resolve conflicts?

Hi @andrewaikens87, please re-review if you have a chance. Or open a new issue with requested changes if I merge it before you can review.

@andrewaikens87

This comment has been minimized.

Copy link
Member

andrewaikens87 commented Jul 16, 2018

Yes I will re-review once the merge conflicts are resolved.

@bmcutler bmcutler merged commit 53dd133 into master Jul 17, 2018

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@bmcutler bmcutler deleted the forum_thread_resolve_status branch Jul 18, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment