Easyfix #3363: displaying hashtag anchor in url when file link clicked, and scroll with appropriate spacing
Review Request #6782 — Created Jan. 16, 2015 and submitted
Easyfix #3363: displaying hashtag anchor in url when file link clicked, and scroll with appropriate spacing
Changes
- Adjusted "DIFF_SCROLLDOWN_AMOUNT" value to 15 from original 100, which reduces the spaces between top of the element to the browser boundary after jumping to the anchor, avoid leaving a large gap between
- setting the location.hash with the anchor value at the time of scrolling, to replace the part being stripped out by backbone
- manage.py test passed
- created open reviews with multiple changed files and tested clicking on file links to confirm the url is displaying the correct hashtag and having no extra spaces between the tagged element and top of the browser
- opening new page with link including the hashtag to confirm the hashtag remains in place, and the page is scrolled to the correct tagged element location
-
Please list the bug number in the "Bugs" field of the review request.
I'd also like to see some more explanation in your description and testing done. For the description, can you explain what the issue is, and how your change fixes it? In particular, I don't understand what the "scroll with appropriate spacing means"
For the testing done, can you describe exactly the test cases that you manually executed?
JT
- Description:
-
Easyfix #3363: displaying hashtag anchor in url when file link clicked, and scroll with appropriate spacing
+ + Changes
+ - Adjusted "DIFF_SCROLLDOWN_AMOUNT" value to 15 from original 100, which reduces the spaces between top of the element to the browser boundary after jumping to the anchor, avoid leaving a large gap between + - setting the location.hash with the anchor value at the time of scrolling, to replace the part being stripped out by backbone - Testing Done:
-
~ ./reviewboard/manage.py test passed
~ Manually examined the page and confirmed the original issue is fixed ~ - manage.py test passed
~ - created open reviews with multiple changed files and tested clicking on file links to confirm the url is displaying the correct hashtag and having no extra spaces between the tagged element and top of the browser
+ - opening new page with link including the hashtag to confirm the hashtag remains in place, and the page is scrolled to the correct tagged element location
- Bugs:
-
- Added Files:
-
Hey,
Code looks great. I have some notes on review request summaries/descriptions for future posts.
The summary shouldn't contain the bug number. That's already there in the Bugs field. It should however summarize what is fixed, in proper sentence form, without forcing me to know too much about the details of the change. Something high-level that would fit in 80 characters. You can always go into more details in the description.
The description should then go into more details about the reasons/purpose for the change, what you're solving, how you solved it. It should do this without basically repeating the lines of the code.
The goal is for someone unfamiliar with your task to read the summary, description, and testing, and have a solid idea of what's going on with this change at a high level before digging into the code. A good technique is to explain what you're working on to someone/something, as if you're teaching them about it, so that if they were to look at the code, they'd already know the reason for the lines being changed.
Tool: PEP8 Style Checker Ignored Files: reviewboard/static/rb/js/pages/views/diffViewerPageView.js Tool: Pyflakes Ignored Files: reviewboard/static/rb/js/pages/views/diffViewerPageView.js