Implement archive review UI.

Review Request #10794 — Created Nov. 9, 2019 and updated

Information

Review Board
master

Reviewers

Currently there is no binary file support for RB, exluding my other
changes. With this arhive files can be posted and reviewed at a
basic level. Reviewers can look through a repersentation of the archive
proposed to be submitted. No support for commonenting or presenting the
actual changes is include in this PR as that was out of scope. The
archive is represented as a JSON file tree and does not have any css or
less changes to make it more unified with the rest of RB. These style
changes will need to be included in a later review request. The JSON
file tree is created using a library called JSTree and is currently
under a MIT license, https://github.com/vakata/jstree/. This feature
does require the changes from my other binary file PRs to function.
Specifically: r/10729, r/10728, and one change from r/10766.

Create a test repo and walked through the steps of a review process.
Posted change, look and interacted with change, then shipped change.
Also wrote tests for parsing the archive files; empty, one level,
and multi level archives.

Summary ID
base framework for archive review ui
f992ac6290d7362341c0b2b8c5c093e819df4d40
Move parsing of archives to backend
730b85ed79a0b37147f1893cf7ae5377ea7c8b95
Implement rendering of archives with JSTree library
4c0eef1642bedf4762de12fe84ea2c2cbe085dde
Add testing and fix some small issues
2ec0d0c00038aa54cf5c8428d63de161210a5e06
address suggestions
7cda64657498e5996b0bb6696e19ba23c42acebd
Address comments
c4de14a0d5e5c0bdaf2c61ad580160e6ef24917a
Address comments
5565f90571992d3ef0383f1e2e8889b416c6dc4c

Description From Last Updated

E302 expected 2 blank lines, found 1

reviewbotreviewbot

F841 local variable 'serialized_comments' is assigned to but never used

reviewbotreviewbot

W292 no newline at end of file

reviewbotreviewbot

Col: 46 Missing semicolon.

reviewbotreviewbot

Col: 29 'obj' is defined but never used.

reviewbotreviewbot

E401 multiple imports on one line

reviewbotreviewbot

E302 expected 2 blank lines, found 1

reviewbotreviewbot

F841 local variable 'serialized_comments' is assigned to but never used

reviewbotreviewbot

W292 no newline at end of file

reviewbotreviewbot

Col: 13 Do not use 'new' for side effects.

reviewbotreviewbot

E501 line too long (95 > 79 characters)

reviewbotreviewbot

E401 multiple imports on one line

reviewbotreviewbot

E302 expected 2 blank lines, found 1

reviewbotreviewbot

F841 local variable 'serialized_comments' is assigned to but never used

reviewbotreviewbot

E265 block comment should start with '# '

reviewbotreviewbot

E303 too many blank lines (2)

reviewbotreviewbot

E226 missing whitespace around arithmetic operator

reviewbotreviewbot

Col: 39 Expected '===' and instead saw '=='.

reviewbotreviewbot

Col: 99 Expected '!==' and instead saw '!='.

reviewbotreviewbot

Col: 173 Expected an assignment or function call and instead saw an expression.

reviewbotreviewbot

Col: 174 Missing semicolon.

reviewbotreviewbot

Col: 677 Expected an assignment or function call and instead saw an expression.

reviewbotreviewbot

Col: 678 Missing semicolon.

reviewbotreviewbot

Col: 716 Missing semicolon.

reviewbotreviewbot

Col: 802 Expected an assignment or function call and instead saw an expression.

reviewbotreviewbot

Col: 803 Missing semicolon.

reviewbotreviewbot

Col: 999 Expected an assignment or function call and instead saw an expression.

reviewbotreviewbot

Col: 1000 Missing semicolon.

reviewbotreviewbot

Col: 1111 Expected '{' and instead saw 'try'.

reviewbotreviewbot

Col: 1121 Missing semicolon.

reviewbotreviewbot

Col: 1149 Expected '{' and instead saw 'try'.

reviewbotreviewbot

Col: 1196 Missing semicolon.

reviewbotreviewbot

Col: 1385 Missing semicolon.

reviewbotreviewbot

Col: 1389 Missing semicolon.

reviewbotreviewbot

Col: 1431 Expected '===' and instead saw '=='.

reviewbotreviewbot

Col: 1500 Confusing use of '!'.

reviewbotreviewbot

Col: 1688 Confusing use of '!'.

reviewbotreviewbot

Col: 1728 Missing semicolon.

reviewbotreviewbot

Col: 1757 Missing semicolon.

reviewbotreviewbot

Col: 1887 Missing semicolon.

reviewbotreviewbot

Col: 1888 Missing semicolon.

reviewbotreviewbot

Col: 2267 Expected an assignment or function call and instead saw an expression.

reviewbotreviewbot

Col: 2268 Missing semicolon.

reviewbotreviewbot

Col: 2331 Expected an assignment or function call and instead saw an expression.

reviewbotreviewbot

Col: 2332 Missing semicolon.

reviewbotreviewbot

Col: 2403 Expected '{' and instead saw 'this'.

reviewbotreviewbot

Col: 2558 Expected an assignment or function call and instead saw an expression.

reviewbotreviewbot

E302 expected 2 blank lines, found 1

reviewbotreviewbot

W292 no newline at end of file

reviewbotreviewbot

E226 missing whitespace around arithmetic operator

reviewbotreviewbot

Col: 39 Expected '===' and instead saw '=='.

reviewbotreviewbot

Col: 99 Expected '!==' and instead saw '!='.

reviewbotreviewbot

Col: 173 Expected an assignment or function call and instead saw an expression.

reviewbotreviewbot

Col: 677 Expected an assignment or function call and instead saw an expression.

reviewbotreviewbot

Col: 678 Missing semicolon.

reviewbotreviewbot

Col: 716 Missing semicolon.

reviewbotreviewbot

Col: 802 Expected an assignment or function call and instead saw an expression.

reviewbotreviewbot

Col: 803 Missing semicolon.

reviewbotreviewbot

Col: 999 Expected an assignment or function call and instead saw an expression.

reviewbotreviewbot

Col: 1000 Missing semicolon.

reviewbotreviewbot

Col: 1111 Expected '{' and instead saw 'try'.

reviewbotreviewbot

Col: 1121 Missing semicolon.

reviewbotreviewbot

Col: 1149 Expected '{' and instead saw 'try'.

reviewbotreviewbot

Col: 1196 Missing semicolon.

reviewbotreviewbot

Col: 174 Missing semicolon.

reviewbotreviewbot

Col: 1385 Missing semicolon.

reviewbotreviewbot

Col: 1389 Missing semicolon.

reviewbotreviewbot

Col: 1431 Expected '===' and instead saw '=='.

reviewbotreviewbot

Col: 1500 Confusing use of '!'.

reviewbotreviewbot

Col: 1688 Confusing use of '!'.

reviewbotreviewbot

Col: 1728 Missing semicolon.

reviewbotreviewbot

Col: 1757 Missing semicolon.

reviewbotreviewbot

Col: 1888 Missing semicolon.

reviewbotreviewbot

Col: 2267 Expected an assignment or function call and instead saw an expression.

reviewbotreviewbot

Col: 2268 Missing semicolon.

reviewbotreviewbot

Col: 2331 Expected an assignment or function call and instead saw an expression.

reviewbotreviewbot

Col: 2332 Missing semicolon.

reviewbotreviewbot

Col: 2403 Expected '{' and instead saw 'this'.

reviewbotreviewbot

Col: 2558 Expected an assignment or function call and instead saw an expression.

reviewbotreviewbot

Col: 1887 Missing semicolon.

reviewbotreviewbot

The rest of this file is sorted.

brenniebrennie

I think you copied this docstring and did update it.

brenniebrennie

Missing docstring.

brenniebrennie

Missing docstring.

brenniebrennie

Missing docstring.

brenniebrennie

Missing docstring.

brenniebrennie

Missing file-level docstring.

brenniebrennie

Missing class docstring.

brenniebrennie

Missing docstring.

brenniebrennie

TODO.

brenniebrennie

nit: blank line between these.

brenniebrennie

Missing docstring.

brenniebrennie

nit: blank line between these.

brenniebrennie

Missing docstring. Typo: "recurse"

brenniebrennie

nit: blank line between these.

brenniebrennie

nit: add a trailing comma

brenniebrennie

Col: 39 Expected '===' and instead saw '=='.

reviewbotreviewbot

Col: 99 Expected '!==' and instead saw '!='.

reviewbotreviewbot

Col: 173 Expected an assignment or function call and instead saw an expression.

reviewbotreviewbot

Col: 174 Missing semicolon.

reviewbotreviewbot

Col: 677 Expected an assignment or function call and instead saw an expression.

reviewbotreviewbot

Col: 678 Missing semicolon.

reviewbotreviewbot

Col: 716 Missing semicolon.

reviewbotreviewbot

Col: 802 Expected an assignment or function call and instead saw an expression.

reviewbotreviewbot

Col: 803 Missing semicolon.

reviewbotreviewbot

Col: 999 Expected an assignment or function call and instead saw an expression.

reviewbotreviewbot

Col: 1000 Missing semicolon.

reviewbotreviewbot

Col: 1111 Expected '{' and instead saw 'try'.

reviewbotreviewbot

Col: 1121 Missing semicolon.

reviewbotreviewbot

Col: 1149 Expected '{' and instead saw 'try'.

reviewbotreviewbot

Col: 1196 Missing semicolon.

reviewbotreviewbot

Col: 1385 Missing semicolon.

reviewbotreviewbot

Col: 1389 Missing semicolon.

reviewbotreviewbot

Col: 1431 Expected '===' and instead saw '=='.

reviewbotreviewbot

Col: 1500 Confusing use of '!'.

reviewbotreviewbot

Col: 1688 Confusing use of '!'.

reviewbotreviewbot

Col: 1728 Missing semicolon.

reviewbotreviewbot

Col: 1757 Missing semicolon.

reviewbotreviewbot

Col: 1887 Missing semicolon.

reviewbotreviewbot

Col: 1888 Missing semicolon.

reviewbotreviewbot

Col: 2267 Expected an assignment or function call and instead saw an expression.

reviewbotreviewbot

Col: 2268 Missing semicolon.

reviewbotreviewbot

Col: 2331 Expected an assignment or function call and instead saw an expression.

reviewbotreviewbot

Col: 2332 Missing semicolon.

reviewbotreviewbot

Col: 2403 Expected '{' and instead saw 'this'.

reviewbotreviewbot

Col: 2558 Expected an assignment or function call and instead saw an expression.

reviewbotreviewbot

We don't need to specify empty initialize methods

brenniebrennie

nit: remove this line.

brenniebrennie

nit: remove this line.

brenniebrennie

Leftover debug logging?

brenniebrennie

Since reviewboard already uses fontawesome which has its own loading spinners, it may be better to replace the occurrences of …

amalik2amalik2

I'm most likely wrong about this since I know very little about licensing but I think that even for permissive …

amalik2amalik2

Col: 39 Expected '===' and instead saw '=='.

reviewbotreviewbot

Col: 99 Expected '!==' and instead saw '!='.

reviewbotreviewbot

Col: 173 Expected an assignment or function call and instead saw an expression.

reviewbotreviewbot

Col: 174 Missing semicolon.

reviewbotreviewbot

Col: 677 Expected an assignment or function call and instead saw an expression.

reviewbotreviewbot

Col: 678 Missing semicolon.

reviewbotreviewbot

Col: 716 Missing semicolon.

reviewbotreviewbot

Col: 802 Expected an assignment or function call and instead saw an expression.

reviewbotreviewbot

Col: 803 Missing semicolon.

reviewbotreviewbot

Col: 999 Expected an assignment or function call and instead saw an expression.

reviewbotreviewbot

Col: 1000 Missing semicolon.

reviewbotreviewbot

Col: 1111 Expected '{' and instead saw 'try'.

reviewbotreviewbot

Col: 1121 Missing semicolon.

reviewbotreviewbot

Col: 1149 Expected '{' and instead saw 'try'.

reviewbotreviewbot

Col: 1196 Missing semicolon.

reviewbotreviewbot

Col: 1385 Missing semicolon.

reviewbotreviewbot

Col: 1389 Missing semicolon.

reviewbotreviewbot

Col: 1431 Expected '===' and instead saw '=='.

reviewbotreviewbot

Col: 1500 Confusing use of '!'.

reviewbotreviewbot

Col: 1688 Confusing use of '!'.

reviewbotreviewbot

Col: 1728 Missing semicolon.

reviewbotreviewbot

Col: 1757 Missing semicolon.

reviewbotreviewbot

Col: 1887 Missing semicolon.

reviewbotreviewbot

Col: 1888 Missing semicolon.

reviewbotreviewbot

Col: 2267 Expected an assignment or function call and instead saw an expression.

reviewbotreviewbot

Col: 2268 Missing semicolon.

reviewbotreviewbot

Col: 2331 Expected an assignment or function call and instead saw an expression.

reviewbotreviewbot

Col: 2332 Missing semicolon.

reviewbotreviewbot

Col: 2403 Expected '{' and instead saw 'this'.

reviewbotreviewbot

Col: 2558 Expected an assignment or function call and instead saw an expression.

reviewbotreviewbot

diffAgainstArchiveMetadata should also be mentioned in the docstring

amalik2amalik2

Should this be mode? The subclasses of BaseArchiveView define mode instead of node.

amalik2amalik2

Did you define CSS classes anywhere for the different subclasses of BaseArchiveView? I can't find anything by doing a search …

amalik2amalik2

I'm curious as to how this works... does jstree modify the String prototype to add a jstree method to it?

amalik2amalik2

Single quotes should be used

amalik2amalik2

Where/how is this used? I don't see any usages in this file

amalik2amalik2

Where/how is this used? I don't see any usages in this file

amalik2amalik2

Where/how is this used? I don't see any usages in this file

amalik2amalik2

What is this used for?

amalik2amalik2

Is there a revision selector slider that gets rendered somewhere? (the text/image based review UIs have it)

amalik2amalik2

Not a huge deal but in general, using Boolean(value) is easier to understand than !!value

amalik2amalik2

Wouldn't this crash if there is a diff type mismatch? this._archiveView looks like it would be undefined in that situation.

amalik2amalik2

To be more consistent with the other CSS files, it would be better to rename the css file to something …

amalik2amalik2

Col: 39 Expected '===' and instead saw '=='.

reviewbotreviewbot

Col: 99 Expected '!==' and instead saw '!='.

reviewbotreviewbot

Col: 173 Expected an assignment or function call and instead saw an expression.

reviewbotreviewbot

Col: 174 Missing semicolon.

reviewbotreviewbot

Col: 677 Expected an assignment or function call and instead saw an expression.

reviewbotreviewbot

Col: 678 Missing semicolon.

reviewbotreviewbot

Col: 716 Missing semicolon.

reviewbotreviewbot

Col: 802 Expected an assignment or function call and instead saw an expression.

reviewbotreviewbot

Col: 803 Missing semicolon.

reviewbotreviewbot

Col: 999 Expected an assignment or function call and instead saw an expression.

reviewbotreviewbot

Col: 1000 Missing semicolon.

reviewbotreviewbot

Col: 1111 Expected '{' and instead saw 'try'.

reviewbotreviewbot

Col: 1121 Missing semicolon.

reviewbotreviewbot

Col: 1149 Expected '{' and instead saw 'try'.

reviewbotreviewbot

Col: 1196 Missing semicolon.

reviewbotreviewbot

Col: 1385 Missing semicolon.

reviewbotreviewbot

Col: 1389 Missing semicolon.

reviewbotreviewbot

Col: 1431 Expected '===' and instead saw '=='.

reviewbotreviewbot

Col: 1500 Confusing use of '!'.

reviewbotreviewbot

Col: 1688 Confusing use of '!'.

reviewbotreviewbot

Col: 1728 Missing semicolon.

reviewbotreviewbot

Col: 1757 Missing semicolon.

reviewbotreviewbot

Col: 1887 Missing semicolon.

reviewbotreviewbot

Col: 1888 Missing semicolon.

reviewbotreviewbot

Col: 2267 Expected an assignment or function call and instead saw an expression.

reviewbotreviewbot

Col: 2268 Missing semicolon.

reviewbotreviewbot

Col: 2331 Expected an assignment or function call and instead saw an expression.

reviewbotreviewbot

Col: 2332 Missing semicolon.

reviewbotreviewbot

Col: 2403 Expected '{' and instead saw 'this'.

reviewbotreviewbot

Col: 2558 Expected an assignment or function call and instead saw an expression.

reviewbotreviewbot
Checks run (2 failed)
flake8 failed.
JSHint failed.

flake8

JSHint

PE
Review request changed
Commits:
Summary ID
base framework for archive review ui
8cde8e072e6ecb8b89fbbf5f1c8dff80c79d4482
base framework for archive review ui
a124db6397d047737a4c049f59ba4e667963bfbe
Move parsing of archives to backend
8dd1f82aaca9f468f7dc25a8be8b2b87677961a1

Checks run (2 failed)

flake8 failed.
JSHint failed.

flake8

JSHint

PE
Review request changed
Commits:
Summary ID
base framework for archive review ui
a124db6397d047737a4c049f59ba4e667963bfbe
Move parsing of archives to backend
8dd1f82aaca9f468f7dc25a8be8b2b87677961a1
base framework for archive review ui
a124db6397d047737a4c049f59ba4e667963bfbe
Move parsing of archives to backend
8dd1f82aaca9f468f7dc25a8be8b2b87677961a1
Implement rendering of archives with JSTree library
ff10db2261cb13ad5e52bd56240b8cce0564e70e

Checks run (2 failed)

flake8 failed.
JSHint failed.

flake8

JSHint

PE
Review request changed
Summary:
WIP: Archive Review UI
Archive Review UI
Description:
~  

base framework for archive review ui

  ~

Currently there is no binary file support for RB, exluding my other changes. With this arhive files can be posted and reviewed at a basic level. Reviewers can look through a repersentation of the archive proposed to be submitted. No support for commonenting or presenting the actual changes is include in this PR as that was out of scope. The archive is represented as a JSON file tree and does not have any css or less changes to make it more unified with the rest of RB. These style changes will need to be included in a later review request. The JSON file tree is created using a library called JSTree and is currently under a MIT license, https://github.com/vakata/jstree/. This feature does require the changes from my other binary file PRs to function. Specifically: r/10729, r/10728, and one change from r/10766.

Testing Done:
  +

Create a test repo and walked through the steps of a review process. Posted change, look and interacted with change, then shipped change. Also wrote tests for parsing the archive files; empty, one level, and multi level archives.

Commits:
Summary ID
base framework for archive review ui
a124db6397d047737a4c049f59ba4e667963bfbe
Move parsing of archives to backend
8dd1f82aaca9f468f7dc25a8be8b2b87677961a1
Implement rendering of archives with JSTree library
ff10db2261cb13ad5e52bd56240b8cce0564e70e
base framework for archive review ui
f992ac6290d7362341c0b2b8c5c093e819df4d40
Move parsing of archives to backend
730b85ed79a0b37147f1893cf7ae5377ea7c8b95
Implement rendering of archives with JSTree library
4c0eef1642bedf4762de12fe84ea2c2cbe085dde
Add testing and fix some small issues
2ec0d0c00038aa54cf5c8428d63de161210a5e06

Checks run (2 failed)

flake8 failed.
JSHint failed.

flake8

JSHint

PE
Review request changed
Commits:
Summary ID
base framework for archive review ui
f992ac6290d7362341c0b2b8c5c093e819df4d40
Move parsing of archives to backend
730b85ed79a0b37147f1893cf7ae5377ea7c8b95
Implement rendering of archives with JSTree library
4c0eef1642bedf4762de12fe84ea2c2cbe085dde
Add testing and fix some small issues
2ec0d0c00038aa54cf5c8428d63de161210a5e06
base framework for archive review ui
f992ac6290d7362341c0b2b8c5c093e819df4d40
Move parsing of archives to backend
730b85ed79a0b37147f1893cf7ae5377ea7c8b95
Implement rendering of archives with JSTree library
4c0eef1642bedf4762de12fe84ea2c2cbe085dde
Add testing and fix some small issues
2ec0d0c00038aa54cf5c8428d63de161210a5e06
address suggestions
7cda64657498e5996b0bb6696e19ba23c42acebd

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

PE
PE
brennie
  1. 
      
  2. dev-requirements.txt (Diff revision 5)
     
     
    Show all issues

    The rest of this file is sorted.

  3. Show all issues

    I think you copied this docstring and did update it.

  4. Show all issues

    Missing docstring.

  5. Show all issues

    Missing docstring.

  6. Show all issues

    Missing docstring.

  7. Show all issues

    Missing docstring.

  8. reviewboard/reviews/ui/archive.py (Diff revision 5)
     
     
    Show all issues

    Missing file-level docstring.

  9. reviewboard/reviews/ui/archive.py (Diff revision 5)
     
     
    Show all issues

    Missing class docstring.

  10. reviewboard/reviews/ui/archive.py (Diff revision 5)
     
     
    Show all issues

    Missing docstring.

  11. reviewboard/reviews/ui/archive.py (Diff revision 5)
     
     
    Show all issues

    TODO.

  12. reviewboard/reviews/ui/archive.py (Diff revision 5)
     
     
     
    Show all issues

    nit: blank line between these.

  13. reviewboard/reviews/ui/archive.py (Diff revision 5)
     
     
    Show all issues

    Missing docstring.

  14. reviewboard/reviews/ui/archive.py (Diff revision 5)
     
     
     
    Show all issues

    nit: blank line between these.

  15. reviewboard/reviews/ui/archive.py (Diff revision 5)
     
     
    Show all issues

    Missing docstring.

    Typo: "recurse"

  16. reviewboard/reviews/ui/archive.py (Diff revision 5)
     
     
     
    Show all issues

    nit: blank line between these.

  17. reviewboard/reviews/ui/archive.py (Diff revision 5)
     
     
    Show all issues

    nit: add a trailing comma

  18. Show all issues

    We don't need to specify empty initialize methods

  19. Show all issues

    nit: remove this line.

  20. Show all issues

    nit: remove this line.

  21. Show all issues

    Leftover debug logging?

  22. 
      
PE
Review request changed
Commits:
Summary ID
base framework for archive review ui
f992ac6290d7362341c0b2b8c5c093e819df4d40
Move parsing of archives to backend
730b85ed79a0b37147f1893cf7ae5377ea7c8b95
Implement rendering of archives with JSTree library
4c0eef1642bedf4762de12fe84ea2c2cbe085dde
Add testing and fix some small issues
2ec0d0c00038aa54cf5c8428d63de161210a5e06
address suggestions
7cda64657498e5996b0bb6696e19ba23c42acebd
base framework for archive review ui
f992ac6290d7362341c0b2b8c5c093e819df4d40
Move parsing of archives to backend
730b85ed79a0b37147f1893cf7ae5377ea7c8b95
Implement rendering of archives with JSTree library
4c0eef1642bedf4762de12fe84ea2c2cbe085dde
Add testing and fix some small issues
2ec0d0c00038aa54cf5c8428d63de161210a5e06
address suggestions
7cda64657498e5996b0bb6696e19ba23c42acebd
Address comments
c4de14a0d5e5c0bdaf2c61ad580160e6ef24917a

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

amalik2
  1. 
      
  2. Show all issues

    Since reviewboard already uses fontawesome which has its own loading spinners, it may be better to replace the occurrences of background:url("throbber.gif") in this CSS file with the fontawesome equivalent. That way, you wouldn't need to add in the throbber.gif file

    1. I don't want to diverge the local copy of the library from what is being produced.

  3. reviewboard/static/lib/js/jstree.min.js (Diff revision 6)
     
     
    Show all issues

    I'm most likely wrong about this since I know very little about licensing but I think that even for permissive licenses like MIT, you need to put the entire license text where you use the code. You probably should verify this with the mentors

    1. From what I understand MIT licenses only require the license where the software is not where it is used.

      https://opensource.org/licenses/MIT

  4. Show all issues

    diffAgainstArchiveMetadata should also be mentioned in the docstring

  5. Show all issues

    Should this be mode? The subclasses of BaseArchiveView define mode instead of node.

  6. Show all issues

    Did you define CSS classes anywhere for the different subclasses of BaseArchiveView? I can't find anything by doing a search for 'archive-diff-'

    1. As stated in the description and in the demo video alot of work will need to be done to actually bring a more unifed look to the archive diffing. For that reason I'm not including style changes at this time.

  7. Show all issues

    I'm curious as to how this works... does jstree modify the String prototype to add a jstree method to it?

  8. Show all issues

    Single quotes should be used

  9. Show all issues

    Where/how is this used? I don't see any usages in this file

  10. Show all issues

    Where/how is this used? I don't see any usages in this file

  11. Show all issues

    Where/how is this used? I don't see any usages in this file

  12. Show all issues

    What is this used for?

  13. Show all issues

    Is there a revision selector slider that gets rendered somewhere? (the text/image based review UIs have it)

    1. I believe that is handled by the file attachment class. The mentions to a slider in a the image reviewable are for the onion and diffing sliders that overlap images.

    2. https://github.com/reviewboard/reviewboard/blob/master/reviewboard/static/rb/js/views/imageReviewableView.es6.js#L990

      I had to explicitly include this in my code also to get it to show up. You can test this by creating a file attachment and updating it. When viewing details about the attachment, it should show the revision selector in the review header

  14. Show all issues

    Not a huge deal but in general, using Boolean(value) is easier to understand than !!value

    1. This the same as the image reviewable view.

  15. Show all issues

    Wouldn't this crash if there is a diff type mismatch? this._archiveView looks like it would be undefined in that situation.

  16. reviewboard/staticbundles.py (Diff revision 6)
     
     
    Show all issues

    To be more consistent with the other CSS files, it would be better to rename the css file to something like jstree.min.css, and then move it outside of the jstree folder (and remove the jstree folder altogether)

    1. This is consistent with what is done in the JS folder. Look at the flot folder in static/lib/js.

  17. 
      
PE
Review request changed
Commits:
Summary ID
base framework for archive review ui
f992ac6290d7362341c0b2b8c5c093e819df4d40
Move parsing of archives to backend
730b85ed79a0b37147f1893cf7ae5377ea7c8b95
Implement rendering of archives with JSTree library
4c0eef1642bedf4762de12fe84ea2c2cbe085dde
Add testing and fix some small issues
2ec0d0c00038aa54cf5c8428d63de161210a5e06
address suggestions
7cda64657498e5996b0bb6696e19ba23c42acebd
Address comments
c4de14a0d5e5c0bdaf2c61ad580160e6ef24917a
base framework for archive review ui
f992ac6290d7362341c0b2b8c5c093e819df4d40
Move parsing of archives to backend
730b85ed79a0b37147f1893cf7ae5377ea7c8b95
Implement rendering of archives with JSTree library
4c0eef1642bedf4762de12fe84ea2c2cbe085dde
Add testing and fix some small issues
2ec0d0c00038aa54cf5c8428d63de161210a5e06
address suggestions
7cda64657498e5996b0bb6696e19ba23c42acebd
Address comments
c4de14a0d5e5c0bdaf2c61ad580160e6ef24917a
Address comments
5565f90571992d3ef0383f1e2e8889b416c6dc4c
Added Files:

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint