Skip to content

[TNL-730] View All Notes page. - #5968

Merged
tymofij merged 14 commits into
feature/edxnotesfrom
anton/notes-page
Nov 20, 2014
Merged

[TNL-730] View All Notes page.#5968
tymofij merged 14 commits into
feature/edxnotesfrom
anton/notes-page

Conversation

@polesye

@polesye polesye commented Nov 17, 2014

Copy link
Copy Markdown
Contributor
  • View all notes made by the user in the courseware
  • Page appears in the top level course navigation, labeled "Notes"
  • Notes should be accompanied by their highlighted context.
  • Default sort for notes should be created/edited date of the note, in descending order.

https://openedx.atlassian.net/browse/TNL-730

@tymofij , @olmar , @jmclaus please review.

sandbox: http://polesye.m.sandbox.edx.org/courses/edX/DemoX/Demo_Course/edxnotes

@polesye
polesye force-pushed the anton/notes-page branch 2 times, most recently from 5e94581 to 0e0adc3 Compare November 18, 2014 12:54

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@polesye It should be descending not ascending.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@polesye Actually, I take that back. Now that comparator has been removed from the notes collection, we are not testing ordering in Jasmine as per say. Just that the order in which models are added are unchanged in the rendering. So I would say

it('should display notes in the same order as they were added as well as their corresponding text and quote'...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Actually, I take that back. Now that comparator has been removed from the notes collection, we are not testing ordering in Jasmine as per say. Just that the order in which models are added are unchanged in the rendering. So I would say

it('should display notes in the same order as they were added as well as their corresponding text and quote'...

Done.

@jmclaus

jmclaus commented Nov 19, 2014

Copy link
Copy Markdown

@polesye Regarding the JS part 👍 when the above comments are addressed.

@explorerleslie

Copy link
Copy Markdown

@polesye because I can't save any notes in your sandbox, I can't actually see the Notes page with anything meaningful on it. Before I can thumbs up, I need to be able to see this. Is there anyway to hook up the backend so that we can save some notes and I can see the Notes page? @olmar or @tymofij ?

Comment thread lms/djangoapps/edxnotes/views.py Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Seems docstring should be not in one line but formatted to a multi-line docstring:

"""
Displays the EdxNotes page.
"""

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Seems docstring should be not in one line but formatted to a multi-line docstring:

Done.

@jmclaus

jmclaus commented Nov 20, 2014

Copy link
Copy Markdown

@polesye 👍 Once tests pass.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please make multi-line docstring.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Please make multi-line docstring.

Fixed.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please make multi-line docstring.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Please make multi-line docstring.

Fixed.

@polesye

polesye commented Nov 20, 2014

Copy link
Copy Markdown
Contributor Author

@olmar your comments are addressed.

@tymofij tymofij changed the title [WIP] Notes page. [TNL-730] View All Notes page. Nov 20, 2014
tymofij added a commit that referenced this pull request Nov 20, 2014
[TNL-730] View All Notes page.
@tymofij
tymofij merged commit 977bfc1 into feature/edxnotes Nov 20, 2014
@benpatterson
benpatterson deleted the anton/notes-page branch August 2, 2016 13:26
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.

5 participants