Apache Allura™ / Git / Merge Request #131: [#8088] Design changes to Discussions (merged)

Dave Brondsema - 2016-06-17

I've looked over the code, and it looks pretty good to me. No concerns at this point. Haven't had a chance to test it out directly yet.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Rohan Verma - 2016-06-20

Hey,

Unable to fix the problems with some of the tests. I can't isolate what is causing it.

Log Paste: https://forge-allura.apache.org/p/allura/pastebin/5767bd1f6d19cd0927da58f9

The paste contains some of the tests that are failing due to the same issue.

Last edit: Rohan Verma 2016-06-20

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
- Dave Brondsema - 2016-06-21
  
  In commit [221d55] you removed a </form> closing tag, probably that's it?
  
  <form method="POST" class="moderate_spam" action="{{action}}"> <input type="hidden" name="spam" value="True"/> - <a href="" class="moderate_post little_link"><span>Spam</span></a> + <a href="" class="moderate_post little_link icon btn ui-button ui-widget ui-state-default ui-corner-all ui-button-text-only"><span><i class="fa fa-exclamation" aria-hidden="true"></i> Spam</span></a> {{lib.csrf_token()}} - </form>
  
  If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
  - Rohan Verma - 2016-06-21
    
    Thanks for pointing that out. I totally missed that.
    
    I think this is now passing all tests on my machine. :)
    
    Last edit: Rohan Verma 2016-06-21
    
    If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Rohan Verma - 2016-06-20

Description:

Diff:

--- old +++ new @@ -1,4 +1,4 @@ Please review. TODO: -* Change tests to check for buttons instead of the words like 'Spam', 'Reply' etc. +* Some tests failing due to form parsing.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Rohan Verma - 2016-06-21

Description:

Diff:

--- old +++ new @@ -1,4 +1 @@ Please review. - -TODO: -* Some tests failing due to form parsing.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Rohan Verma - 2016-06-23

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Rohan Verma - 2016-06-23

Added a style for meta posts as well:

Seems ready for merge.

Last edit: Rohan Verma 2016-06-23

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Dave Brondsema - 2016-06-23

Is the "Tried fixing Tests using replacing reply button with template" commit needed still? Actually it includes a tooltip and the plain HTML version doesn't. Either way, we should be consistent for the way we do all the buttons in this section. And include tooltips on them.

I get a test failure: allura.tests.functional.test_discuss:TestDiscuss.test_spam_link

The "link" link used to open up a dialog, can we keep that behavior with the new button?

When editing a comment, there's an extra border with space all around it. Doesn't seem very "sleek" :)

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Rohan Verma - 2016-06-24

Fixed the editor:

And re-added the link dialog modal along with tooltips for other buttons.

Removed the unneccessary commit.

Last edit: Rohan Verma 2016-06-24

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Rohan Verma - 2016-06-24

Updated test.

Last edit: Rohan Verma 2016-06-24

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Dave Brondsema - 2016-06-29

I think there are too few borders now when editing :) Hard to see where the text area is. I think it should look basically the same whether you're editing a comment or making a new comment. Attached is an example of each.

2016-06-29_1605_edit_border.png

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
- Dave Brondsema - 2016-06-29
  
  That's in Firefox. It looks like Chrome is a little different, but still missing some of the lines around the toolbar and status bar (where it says "lines" and "words")
  
  If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Also some test failures:

======================================================================
FAIL: forgetracker.tests.functional.test_root.TestFunctionalController.test_comment_split
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/brondsem/sf/allura/env-osx/lib/python2.7/site-packages/nose/case.py", line 197, in runTest
    self.test(*self.arg)
  File "/Users/brondsem/sf/allura/ForgeTracker/forgetracker/tests/functional/test_root.py", line 1469, in test_comment_split
    assert_true(len(r.html.findAll(attrs={'class': 'discussion-post'})) == 1)
AssertionError: False is not true
-------------------- >> begin captured stdout << ---------------------
Running setup_app() from allura.websetup

--------------------- >> end captured stdout << ----------------------

======================================================================
FAIL: forgetracker.tests.functional.test_root.TestFunctionalController.test_move_ticket_feed
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/brondsem/sf/allura/env-osx/lib/python2.7/site-packages/nose/case.py", line 197, in runTest
    self.test(*self.arg)
  File "/Users/brondsem/sf/allura/Allura/allura/tests/decorators.py", line 82, in wrapped
    return func(*args, **kw)
  File "/Users/brondsem/sf/allura/ForgeTracker/forgetracker/tests/functional/test_root.py", line 2149, in test_move_ticket_feed
    assert_equal(comments_cnt, 2)  # moved auto comment + new comment
AssertionError: 0 != 2

Rohan Verma - 2016-07-06

Fixed the Borders in the editor and the failed tests due to the new CSS class

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Rohan Verma - 2016-07-11

Hey, wanted to ask a doubt had 2 merge requests that I wanted to share but they were branched from this branch. How would I do that?

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
- Dave Brondsema - 2016-07-11
  
  Go ahead and make new merge requests for them, and just comment that they were branched from this one, that should be fine. Then I'll know to ignore the previous commits from this part, when I review and merge it.
  
  If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Dave Brondsema - 2016-07-11

Found one more issue while testing it again :( Only on one specific location though. When editing an existing ticket, the comment box at the bottom has some layout issues and is missing some borders.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
- Rohan Verma - 2016-07-12
  
  Have updated this by hiding the reply box
  
  If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
  - Dave Brondsema - 2016-07-12
    
    The comment box is useful though, so you can add a comment while you're editing a ticket. I think we should keep it and make it look okay.
    
    I'm not sure what's making it look funky, but maybe we could reduce some of the differences on that page so it looks correct without having to add many specific rules to fix the layout.
    
    If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
    - Rohan Verma - 2016-07-12
      
      Fixed this 👍
      
      If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Dave Brondsema - 2016-07-13

Status: open --> merged
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Dave Brondsema - 2016-07-13

Awesome to see this completely working now, it's going be nice to use. (We might be able to refine it a bit more in the future too, seems that the comments could be a bit wider since there is extra whitespace on the left where the author info is)

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Apache Allura™

Forge software for hosting software projects

Branches

Tags

Git Merge Request #131: [#8088] Design changes to Discussions (merged)

Merging...

Merged

Something went wrong. Please, merge manually

Checking if merge is possible...

Something went wrong. Please, merge manually

Discussion