Review Board Mobile: Review Request Page Box

Review Request #5655 — Created March 24, 2014 and discarded

Information

Review Board
master

Reviewers

Review Board Mobile: Review Request Page Box

Made the review page (/r/:id) responsive, so it can be used on a mobile device
without hassle.

Changes:

  • Viewport zoom is disabled, so that mobile devices will not automatically
    try to adjust the rendering to fit everything into the page. This makes
    the text much easier to read and at its natural size.
  • Action bar (close, update, review, etc.) responds naturally to size, and
    collapses naturally into a pulldown menu bar when there is not enough
    horizontally. Since the number of elements here depends on the state of
    the user/review (e.g. logged in, etc.) and the width of the element is
    unknown prior to rendering, this change is taken care of by javascript.
    It also listens to the resize event, so when orientation is changed on
    the phone, it adapts accordingly.
  • The Information box that is usually on the right handside of the review
    detail box responds to screensize using css media queries. If there is
    not enough space, the box collapses into a toggler which can be used to
    open or close the box. This also saves space for limited screen size on
    mobile
  • Writing reviews and comments is more natural to the mobile interface, as
    the box fits nicely in the screen and the text size is natural. CSS
    media queries have been used to adjust the size and the modal responds
    to the screen size accordingly.
  • The diff viewer does not extrude from the review box and create empty
    white space in the webpage, and fixes the scrolling of the page so the
    user does not go off page. This is done by making the inside content
    scrollable sideways. This user interaction is more natural, because it
    does not get in the way of the user when the user is navigatint the page
    but if necessary the user can scroll to view the diffs.
  • Margins and paddings have been reduced on mobile due to the fact that
    often times the screen size is very limited, and we want to maximise the
    available screen space.

This change generally reflects the user interface standards expected on mobile,
which is to collapse unnecessary content and let content be accessible but not
get in the way of the user.

The changes for the diff viewer can be seen from a video that can be accessed
on the dropbox link below:

https://www.dropbox.com/sh/lryf3dw5ptooss2/UKG6k5xz6G

Tested on:
- iPhone 5S (real hardware)
- desktop browser, changing the screen width
- chrome device emulator (which can emulate different mobile devices)


JO
  1. Did you investigate or figure out that 640px is the limit we should use or is it just for temporary testing?

    1. 640px is the width of horizontal orientation on the iPhone 5/5S. Other than that, there's not much reasoning behind it.

    2. I think if we were only to design for iphone 5/5S and only for portrait use, we could actually use 320px as the limit, as it is the viewport width of iphone 5/5S (see eg. http://viewportsizes.com/?filter=iphone). If we want the mobile UI to be effective also on landscape mode, it would need to be 568px. For one of the larger phones, Galaxy Note 3, the landscape viewport size is 640px. So if we were to target all phones and also landscape modes we could set it to something like that. If we want a mobile UI for some tablets in portrait mode, then it also needs to be higher than the 320px. Also if we want the desktop browser UI to be responsive at more than 320px, the limit would need to be higher. If we end up using a small figure like the 320px, then we probably can make the UI all the way mobile-like (full screen slideout menu drawers etc.) because we don't have to care for the desktop browser experience. In any case we probably should make a variable for the width to be used in the LESS files to modify the value easily. What do you think of all this?

  2. 
      
MI
MI
MI
MI
MI
MI
david
  1. While I appreciate that you seem to be making a lot of progress, this change is very hard to review because it does so many different things. Can you break this up into lots of small changes that each do one thing, that we could potentially commit one at a time?

  2. 
      
MI
JO
    • I think the max viewport width for mobile UI which is now set at 640px and is used in many locations should be a variable (so it can be changed easily if needed). In /r/5620/ I'm using a variable in defs.less for this which we could probably both use.
    • Also seems like the z-index values and the colors in RB should be defined as variables in defs.less.
    • The coding standard (for JavaScript) seems to be 4 spaces per indentation level http://www.reviewboard.org/docs/codebase/dev/coding-standards/.
    • Should we have these JavaScripts in the HTML files inside the script-tags or in separate files?
  1. 
      
MI
Review request changed

Status: Discarded

Loading...