Fixed text fields that have the wrong cursor displayed when hovered over by the cursor

Review Request #8403 — Created Sept. 17, 2016 and submitted

larmiej
Review Board
master
4432
7489e4b...
reviewboard, students

The cursor of some text fields in an edit state displayed the standard
arrow instead of the text cursor when the cursor hovered over the text
fields. Fixed text fields by setting the cursor style to text in the
Code Mirror css to ensure we have the right cursor being displayed to
the user.

  • Opened the review dialog
  • Added a review header
  • Hovered over non-text lines
  • Ensure standard arrow is not diplayed on hover
  • Also, in the review Dialog, Clicked on "Add Comment"
  • Added a single line comment
  • Hovered over non-text lines
  • Ensure standard arrow is not diplayed on hover
Description From Last Updated

Can you wrap your description and testing done at 72 chars?

brenniebrennie

Can you try to limit your summary to 50 chars? (Git doesnt like it when it goes over.)

brenniebrennie

"non-text" in your summary Your summary should also be in the imperitive mood, i.e., "Fix text fields..."

brenniebrennie

This is not the fix we were hoping for. We like the minimum height when the fields are opened. What ...

daviddavid

'formataddr' imported but unused

reviewbotreviewbot

'django_reset' imported but unused

reviewbotreviewbot

'from settings_local import *' used; unable to detect undefined names

reviewbotreviewbot

This file comes from a third party library, and we probably shouldn't make changes to it (since we might drop ...

daviddavid
david
  1. 
      
  2. This is not the fix we were hoping for. We like the minimum height when the fields are opened.

    What we want instead is to change the CSS to ensure we have the right cursor.

  3. Additionally, once you fix that, the summary and description need a lot of love. See https://www.reviewboard.org/docs/codebase/dev/writing-good-descriptions/ for an explanation of what we look for in these, along with examples.

chipx86
  1. Something that might be making this confusing to diagnose is that there's a regression in the current release-3.0.x branch where the last text field on the review request page isn't taking up the correct height (it's supposed to fill the available height). Most text fields won't show this issue.

    What you can do is go to the review dialog and add a review text header. Add a few words on one line. Note how the mouse cursor, when hovering over the first line, has a standard "I" cursor, but when hovering on the following lines (where there's not yet any text), the cursor is an arrow.

    What you should be doing in this change is ensuring that the appropriate element in the text field (which uses CodeMirror, a text editor component) has the correct cursor type (text) by default. You'll have to investigate the elements that comprise the field and try to find the most suitable parent element for this. You won't want to guess, but rather try to determine what each element means and is for, and put the CSS rule in the correct place for it.

  2. 
      
LA
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/reviews/ui/image.py
        reviewboard/notifications/tests.py
        reviewboard/extensions/tests.py
        reviewboard/reviews/ui/tests.py
        reviewboard/accounts/trophies.py
        reviewboard/diffviewer/tests.py
        reviewboard/diffviewer/diffutils.py
        reviewboard/settings.py
        reviewboard/notifications/email.py
        reviewboard/staticbundles.py
        reviewboard/accounts/forms/auth.py
    
    Ignored Files:
        reviewboard/static/rb/css/pages/image-review-ui.less
        reviewboard/static/rb/js/views/regionCommentBlockView.es6.js
        reviewboard/static/rb/js/models/imageReviewableModel.es6.js
        reviewboard/static/lib/css/codemirror.css
        reviewboard/static/rb/css/pages/review-request.less
        reviewboard/static/rb/js/models/imageReviewableModel.js
        reviewboard/static/rb/js/views/imageReviewableView.es6.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/testing/testcase.py
        reviewboard/reviews/ui/image.py
        reviewboard/notifications/tests.py
        reviewboard/extensions/tests.py
        reviewboard/reviews/ui/tests.py
        reviewboard/accounts/trophies.py
        reviewboard/diffviewer/tests.py
        reviewboard/diffviewer/diffutils.py
        reviewboard/settings.py
        reviewboard/notifications/email.py
        reviewboard/staticbundles.py
        reviewboard/accounts/forms/auth.py
    
    Ignored Files:
        reviewboard/static/rb/css/pages/image-review-ui.less
        reviewboard/static/rb/js/views/regionCommentBlockView.es6.js
        reviewboard/static/rb/js/models/imageReviewableModel.es6.js
        reviewboard/static/lib/css/codemirror.css
        reviewboard/static/rb/css/pages/review-request.less
        reviewboard/static/rb/js/models/imageReviewableModel.js
        reviewboard/static/rb/js/views/imageReviewableView.es6.js
    
    
    1. Once you've fixed up the issues from Review Bot, make sure to mark them as fixed or dropped (when appropriate).

  2. reviewboard/notifications/email.py (Diff revision 2)
     
     
     'formataddr' imported but unused
    
  3. reviewboard/settings.py (Diff revision 2)
     
     
     'django_reset' imported but unused
    
  4. reviewboard/settings.py (Diff revision 2)
     
     
     'from settings_local import *' used; unable to detect undefined names
    
  5. 
      
chipx86
  1. You'll need to make sure your branch is cleanly rebased onto master. Don't pull master into your branch. Rather, do:

    $ git checkout master
    $ git pull origin master
    $ git checkout your-feature-branch
    $ git rebase master
    

    If you pulled master into your branch, you might have some issues cleaning some of that up. If so, come to us in Slack for help.

  2. 
      
LA
reviewbot
  1. Tool: Pyflakes
    Ignored Files:
        reviewboard/static/lib/css/codemirror.css
    
    
    
    Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/lib/css/codemirror.css
    
    
  2. 
      
brennie
  1. Can you wra

  2. Can you wrap your description and testing done at 72 chars?

  3. Can you try to limit your summary to 50 chars? (Git doesnt like it when it goes over.)

  4. 
      
LA
chipx86
  1. The code looks okay I believe (though there might be a more specific CSS selector that should be used for this), but the Description should be able to tell a story about what the problem is, so that a person doesn't have to go to the bug report or review request or dig into code.

    I like to follow a "What was wrong, why, and what did we do about it" form. We have a guide and examples here: https://www.reviewboard.org/docs/codebase/dev/writing-good-descriptions/

  2. 
      
LA
LA
LA
david
  1. 
      
  2. This file comes from a third party library, and we probably shouldn't make changes to it (since we might drop in a replacement when we update the version of CodeMirror that we use).

    We have some style overrides for .CodeMirror in reviewboard/static/rb/css/pages/reviews.less. Can you make this change there, instead?

  3. 
      
LA
reviewbot
  1. Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/css/pages/reviews.less
    
    
    
    Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/css/pages/reviews.less
    
    
  2. 
      
brennie
  1. 
      
  2. "non-text" in your summary

    Your summary should also be in the imperitive mood, i.e., "Fix text fields..."

    1. It doesn't look like you updated the summary as requested.

    2. Yeah, I realized I updated my description instead. Thanks for catching that @David

  3. 
      
LA
LA
brennie
  1. Ship It!
  2. 
      
LA
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-2.5.x (8c043ea)
Loading...