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

Rohan Verma wants to merge 18 commits from /u/rhnvrm/allura/ to master, 2016-07-13

Please review.

Commit Date  
[03dcc3] by Rohan Verma Rohan Verma

[#8088] Fixed design issue in codemirror while editing ticket

 2016-07-12 21:09:26 Tree
[922c1b] by Rohan Verma Rohan Verma

[#8088] Hide discussion reply while editing ticket

 2016-07-12 18:47:21 Tree
[4581b9] by Rohan Verma Rohan Verma

[#8088] Fixed Test failiures due to new css meta_post class for discussions

 2016-07-03 20:03:12 Tree
[e0759f] by Rohan Verma Rohan Verma

[#8088] Fixed design changes on editor

 2016-07-01 07:40:22 Tree
[23477b] by Rohan Verma Rohan Verma

[#8088] Update test to match new HTML

 2016-06-24 15:23:57 Tree
[717011] by Rohan Verma Rohan Verma

[#8088] Updated edit button style to match new design changes. minor design changes

 2016-06-24 15:10:58 Tree
[e16c65] by Rohan Verma Rohan Verma

[#8088] Rebased to remove bad commit, Added tooltips to buttons

 2016-06-24 14:35:37 Tree
[f4cebe] by Rohan Verma Rohan Verma

[#8088] Updated style for meta_posts

 2016-06-23 17:28:41 Tree
[0aa1f6] by Rohan Verma Rohan Verma

[#8088] Fix for images exceeding bubble

 2016-06-23 14:04:38 Tree
[10f436] by Rohan Verma Rohan Verma

[#8088] Fix failing tests due to missing closing form tag. Update a test to match new html

 2016-06-21 16:58:00 Tree
[1956ef] by Rohan Verma Rohan Verma

[#8088] test_anonymous_post updated

 2016-06-16 15:52:32 Tree
[55d382] by Rohan Verma Rohan Verma

[#8088] Added check to see if user has permission to reply

 2016-06-09 18:23:54 Tree
[df33e3] by Rohan Verma Rohan Verma

[#8088] Fixed margins

 2016-06-09 16:39:54 Tree
[559fc3] by Rohan Verma Rohan Verma

[#8088] Fix buttons getting cut-off

 2016-06-07 21:37:55 Tree
[d444de] by Rohan Verma Rohan Verma

[#8088] Sleek button design

 2016-06-07 21:13:22 Tree
[8410c1] by Rohan Verma Rohan Verma

[#8088] fix error when deleting comments

 2016-06-04 02:02:44 Tree
[8454a8] by Rohan Verma Rohan Verma

[#8088] fix error on firefox

 2016-06-04 01:32:16 Tree
[221d55] by Rohan Verma Rohan Verma

[#8088] Visual changes to discussion tool. added a header toolbar design

 2016-06-04 01:29:47 Tree

Discussion

  • Dave Brondsema

    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.

     

  • 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

    • Dave Brondsema

      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>
       

      • 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

  • 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 &#39;Spam&#39;, &#39;Reply&#39; etc.
+* Some tests failing due to form parsing.
     

  • Rohan Verma - 2016-06-21
    • Description:

    Diff:

    --- old
+++ new
@@ -1,4 +1 @@
 Please review.
-
-TODO:
-* Some tests failing due to form parsing.
     

  • Rohan Verma - 2016-06-23
     

  • Rohan Verma - 2016-06-23

    Added a style for meta posts as well:

    Imgur

    Seems ready for merge.

     

    Last edit: Rohan Verma 2016-06-23

  • Dave Brondsema

    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" :)

     

  • Rohan Verma - 2016-06-24

    Fixed the editor:
    Imgur

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

    Removed the unneccessary commit.

     

    Last edit: Rohan Verma 2016-06-24

  • Rohan Verma - 2016-06-24

    Updated test.

     

    Last edit: Rohan Verma 2016-06-24

  • Dave Brondsema

    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.

     
    Thumbnail
    2016-06-29_1605_edit_border.png

    • Dave Brondsema

      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")

       

  • Dave Brondsema

    Dave Brondsema - 2016-06-29

    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

     

  • 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?

     

    • Dave Brondsema

      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.

       

  • Dave Brondsema

    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.

     

    • Rohan Verma - 2016-07-12

      Have updated this by hiding the reply box

       

      • Dave Brondsema

        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.

         

        • Rohan Verma - 2016-07-12

          Fixed this 👍

           

  • Dave Brondsema

    Dave Brondsema - 2016-07-13
    • Status: open --> merged
     

  • Dave Brondsema

    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)

     

Log in to post a comment.