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

Forum Multiple Attachements #2243

Merged
merged 21 commits into from Jul 2, 2018
Merged

Conversation

scopeInfinity
Copy link
Contributor

@scopeInfinity scopeInfinity commented Jun 23, 2018

Issue: #2201

Tasks -

  • Multiple Attachments in Create Thread Page
  • Multiple Attachments in Reply Posts
  • Edit Attachments in Edit Posts
  • Thumbnail for attachmens

@scopeInfinity scopeInfinity changed the title [WIP] Forum Multiple Attachements Forum Multiple Attachements Jun 26, 2018
@bmcutler bmcutler requested a review from jn7070 June 28, 2018 19:10
@jn7070
Copy link
Contributor

jn7070 commented Jun 28, 2018

Everything looks good, but is there supposed to be multiple images of the same image even after the message pops up asking if you want to replace it? After pressing submit there is only 1 copy of each image so that works.

test

@andrewaikens87
Copy link
Contributor

@jn7070 That is a good point, also to build on it if you delete one of the two that appear in the list after clicking replace it will not post an image even though there exists an image in the list. I provided some other feedback on slack:

There is a javascript console exception that is thrown when adding in an image:

index.php?semester=s18&course=sample&component=forum&page=view_thread&option=tree&thread_id=9:410 Uncaught TypeError: Cannot read property 'length' of undefined at HTMLDivElement.<anonymous> (index.php?semester=s18&course=sample&component=forum&page=view_thread&option=tree&thread_id=9:410) at HTMLDivElement.dispatch (jquery-2.0.3.min.map.js:5) at HTMLDivElement.y.handle (jquery-2.0.3.min.map.js:5) at HTMLLabelElement.<anonymous> (jquery-2.0.3.min.map.js:5) at init.domManip (jquery-2.0.3.min.map.js:5) at init.prepend (jquery-2.0.3.min.map.js:5) at HTMLDivElement.<anonymous> (index.php?semester=s18&course=sample&component=forum&page=view_thread&option=tree&thread_id=9:419) at HTMLDivElement.dispatch (jquery-2.0.3.min.map.js:5) at HTMLDivElement.y.handle (jquery-2.0.3.min.map.js:5) at addLabel (drag-and-drop.js:289)

Also gave suggestions for the UI to be horizontal rather than vertical (in terms of the list of images) and possibly throwing them inside a bootstrap well or something similar could be nice.

@scopeInfinity
Copy link
Contributor Author

@jn7070 @andrewaikens87 Thankyou for pointing it out.
Fixed it.

Copy link
Contributor

@jn7070 jn7070 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@bmcutler bmcutler merged commit b97ca44 into master Jul 2, 2018
@bmcutler bmcutler deleted the forum_thread_multipleattachments branch July 2, 2018 00:49
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

4 participants