Closed Bug 1309964 Opened 8 years ago Closed 7 years ago

Update Review Request Page Design

Categories

(MozReview Graveyard :: Review Board: User Interface, defect)

Product:
MozReview Graveyard
Component:
Review Board: User Interface
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: davidwalsh, Assigned: davidwalsh)

References

Details

Attachments

(9 files, 5 obsolete files)

MozReview: Review Page Redesign: Move commits table above the main content area (Bug 1309964).
58 bytes, text/x-review-board-request
glob
: review+
smacleod
: review+
Details
MozReview: Review Page Redesign: Update main content area contents (Bug 1309964).
58 bytes, text/x-review-board-request
glob
: review+
smacleod
: review+
Details
MozReview: Review Page Redesign: Enhance table toggling and add cookie table state (Bug 1309964).
58 bytes, text/x-review-board-request
glob
: review+
Details
MozReview: Review Page Redesign: Add user session cookie for commits table persistence (Bug 1309964).
58 bytes, text/x-review-board-request
glob
: review+
Details
MozReview: Review Page Redesign: Add review templates for future 'mozreview-review-content' modification (Bug 1309964).
58 bytes, text/x-review-board-request
glob
: review+
Details
MozReview: Review Page Redesign: Add 'mozreview-review-content' template hook (Bug 1309964)
58 bytes, text/x-review-board-request
glob
: review+
Details
MozReview: Review Page Redesign: Add reviewRequestEditorView_mozreview.js for future height calculation modification (Bug 1309964).
58 bytes, text/x-review-board-request
glob
: review+
Details
MozReview: Review Page Redesign: Prevent reviewboard from calculating main content height (Bug 1309964).
58 bytes, text/x-review-board-request
glob
: review+
Details
MozReview: Review Page Redesign: Update main content area to show full commit message (Bug 1309964).
58 bytes, text/x-review-board-request
glob
: review+
smacleod
: review+
Details 
We've created new mockup designs for the review request and diff pages. This bug represents the work to be done on that front.
Attachment #8800798 - Flags: review?(smacleod)
Comment on attachment 8800798 [details]
MozReview: WIP: Review Page Redesign:  Move commits table above the main content area (Bug 1309964)

https://reviewboard.mozilla.org/r/85642/#review84568

Just clearing the review flag for now.

::: pylib/mozreview/mozreview/extension.py:318
(Diff revision 1)
>                       'mozreview/repository.html')
>          TemplateHook(self, 'base-after-content',
>                       'mozreview/user_review_flag.html',
>                       apply_to=review_request_url_names)
>  
> -        ReviewRequestFieldsHook(self, 'main', [CommitsListField])
> +        MozReviewCommitTableHook(self, 'base-before-content',

Since I'm commenting anyways, I'll leave what we already talked about here as a reminder.

The `ReviewRequestFieldsHook` is important still because the `CommitsListField` takes care of more than just rendering. The field tells Review Board that the commit list data is part of review requests, and needs to be copied from drafts to the review request on publish etc.

::: pylib/mozreview/mozreview/fields.py:101
(Diff revision 1)
>      def should_render(self, value):
>          return is_pushed(self.review_request_details)
>  
> -    def as_html(self):

Just removing this won't be enough, as the default actualy does some rendering. You'll want to make it so `should_render` returns false.
Attachment #8800798 - Flags: review?(smacleod) → review-
Comment on attachment 8800798 [details]
MozReview: WIP: Review Page Redesign:  Move commits table above the main content area (Bug 1309964)

https://reviewboard.mozilla.org/r/85642/#review84664

::: pylib/mozreview/mozreview/hooks.py:52
(Diff revision 2)
> +        # reasons.
> +        children_details = [
> +            child for child in gen_child_rrs(parent_details, user=user)
> +            if child.is_accessible_by(user)]
> +
> +        try:

I know Exceptions are bad for performance -- is there a better way to do this?

::: pylib/mozreview/mozreview/static/mozreview/css/commits.less:31
(Diff revision 2)
>  
>  .help-icon {
>    display: inline-block;
>    text-align: center;
>    background-color: #D1D1D1;
>    border-radius: 50%;

Need to visually mark which review request in the table is the current page's otherwise people can lose where they are.
Comment on attachment 8800798 [details]
MozReview: WIP: Review Page Redesign:  Move commits table above the main content area (Bug 1309964)

https://reviewboard.mozilla.org/r/85642/#review84688

::: pylib/mozreview/mozreview/templates/mozreview/commits-requests.html:38
(Diff revision 4)
> +  {% endif %}
> +
> +
>    {% for child_details in children_details %}
> -  <tr {% if child_details.get_review_request.id = review_request_details.get_review_request.id %}current="true"{% endif %}>
> +  <tr
> +    {% if child_details.get_review_request.id = review_request_details.get_review_request.id %}

Should add a cookie check around here to see if we want to do the "current" stuff or just show the whole thing.
Attachment #8800798 - Attachment is obsolete: true
Attachment #8800798 - Flags: review?(smacleod)
Comment on attachment 8801339 [details]
MozReview: Review Page Redesign: Move commits table above the main content area (Bug 1309964).

https://reviewboard.mozilla.org/r/86118/#review85178

Ummm, I think autoland stuff may be broken with this patch; I need to check that.
Comment on attachment 8801872 [details]
MozReview: Review Page Redesign: Update main content area contents (Bug 1309964).

https://reviewboard.mozilla.org/r/86476/#review85556

::: pylib/mozreview/mozreview/templates/mozreview/review-main.html:21
(Diff revision 1)
> +  <div class="status no-approval" title="{{child_details.get_review_request.approval_failure}}">
> +    r?
> +  </div>
> +{% endif %}
> +
> +<div class="review-request-meta">

This entire block is returning empty strings - wtf?!
Comment on attachment 8801339 [details]
MozReview: Review Page Redesign: Move commits table above the main content area (Bug 1309964).

https://reviewboard.mozilla.org/r/86118/#review85614

::: pylib/mozreview/mozreview/templates/mozreview/commits-requests.html:27
(Diff revision 3)
> +  <tr id="mozreview-child-requests-nav-row">
> +      <td colspan="2">
> +        {% if prev_child %}
> +          <a href="{{prev_child.get_review_request.get_absolute_url}}" rel="previous">&lt;&lt; Previous Commit</a>
> +        {% endif %}
> +        Viewing commit {{current_child_num}} of {{num_children}}. <a href="javascript:;" id="mozreview-all-commits">View All Commits</a>

Must updated this to use cookie to remember open and closed state preference.
Comment on attachment 8801339 [details]
MozReview: Review Page Redesign: Move commits table above the main content area (Bug 1309964).

https://reviewboard.mozilla.org/r/86118/#review87260

comments from playing around with the design (not a code review):

- overall this is very much a step in the right direction

- i find most of the text is way too big, with large amounts of whitespace
     - i need to scroll to see the information i'm interested in
     - in particular changing the size of the code to 13px needs to be reverted
     - #mozreview-review-content is gargantuan, and i think draws too much attention

- i think "view all commits" should look like a button, or be some other way more noticeable
     - it's a very important navigation tool hidden behind a comparatively tiny text link
     - i think it's too easy to miss that a change contains multiple commits

- "view all commits" should be toggleable, with the state stored in a cookie
     - i prefer to always see all commits

- the commit table doesn't have any indication of the currently selected commit, which mixes oddly with next/prev commit navigation controls

- i'd like to see the "finish review" button changed
     - it still looks like a tab :(
     - it's slightly harder to find now because it isn't always at the top of the page
- i'd like to see the actions that are applicable to the whole commit series separated from the per-commit actions
     - ie. the try and autoland links
- in the same neck of the woods, the "download diff" 'button' isn't necessary with the "view raw" link in the commits table, and could be removed

::: pylib/mozreview/mozreview/templates/mozreview/commits.html:32
(Diff revision 3)
> +      {% else %}
> -    <a href="{{parent_details.get_review_request.get_absolute_url}}">Review Summary</a>
> +        <a href="{{parent_details.get_review_request.get_absolute_url}}">Review Summary</a>
> -    <a href="{{parent_details.get_review_request.get_absolute_url}}diff/#index_header">Squashed Diff</a>
> +      {% endif %}
> +      &bull;
> +      {% if review_request_details.get_review_request.bugs_closed %}
> +      <a href="https://bugzilla.mozilla.org/show_bug.cgi?id={{review_request_details.get_review_request.bugs_closed}}">Bug #{{review_request_details.get_review_request.bugs_closed}}</a>

don't hardcode the bugzilla url (it's different on different environments).
Comment on attachment 8801872 [details]
MozReview: Review Page Redesign: Update main content area contents (Bug 1309964).

https://reviewboard.mozilla.org/r/86476/#review87262

::: pylib/mozreview/mozreview/static/mozreview/css/mozilla-theme.less:379
(Diff revision 1)
> -    vertical-align: sub;
> +      vertical-align: sub;
> -  }
> +    }
> -}
> +  }
>  
> +  pre {
> +    font-size: 13px;

please remove this change - being able to see as much code as possible is very important, and this bump from 10px to 13px hinders that.

::: pylib/mozreview/mozreview/templates/mozreview/review-main.html:22
(Diff revision 1)
> +    r?
> +  </div>
> +{% endif %}
> +
> +<div class="review-request-meta">
> +  Authored by {{ review_request_details.get_review_request.author.username }} &bull;

authored by is empty for me.
Comment on attachment 8801339 [details]
MozReview: Review Page Redesign: Move commits table above the main content area (Bug 1309964).

https://reviewboard.mozilla.org/r/86118/#review87352

::: pylib/mozreview/mozreview/templates/mozreview/commits.html:32
(Diff revision 3)
> +      {% else %}
> -    <a href="{{parent_details.get_review_request.get_absolute_url}}">Review Summary</a>
> +        <a href="{{parent_details.get_review_request.get_absolute_url}}">Review Summary</a>
> -    <a href="{{parent_details.get_review_request.get_absolute_url}}diff/#index_header">Squashed Diff</a>
> +      {% endif %}
> +      &bull;
> +      {% if review_request_details.get_review_request.bugs_closed %}
> +      <a href="https://bugzilla.mozilla.org/show_bug.cgi?id={{review_request_details.get_review_request.bugs_closed}}">Bug #{{review_request_details.get_review_request.bugs_closed}}</a>

At one point I found a decorator within the fork that created this link but I can't find it again.
Comment on attachment 8801872 [details]
MozReview: Review Page Redesign: Update main content area contents (Bug 1309964).

https://reviewboard.mozilla.org/r/86476/#review87362

::: pylib/mozreview/mozreview/templates/mozreview/review-main.html:24
(Diff revision 1)
> +{% endif %}
> +
> +<div class="review-request-meta">
> +  Authored by {{ review_request_details.get_review_request.author.username }} &bull;
> +
> +  {% with review_request_details.time_added|date as dateadded and review_request_details.last_updated as lastupdated and review_request_details.last_updated|date:"c" as lastupdatedraw and latest_diffset.timestamp|date:"c" as lastdiffdateraw and latest_diffset.timestamp as lastdiffdate %}

Are there somehow decorators missing that these aren't being output?  I'm confused.
Comment on attachment 8801872 [details]
MozReview: Review Page Redesign: Update main content area contents (Bug 1309964).

https://reviewboard.mozilla.org/r/86476/#review87364

::: pylib/mozreview/mozreview/templates/mozreview/review-main.html:22
(Diff revision 1)
> +    r?
> +  </div>
> +{% endif %}
> +
> +<div class="review-request-meta">
> +  Authored by {{ review_request_details.get_review_request.author.username }} &bull;

From steven -- could be "submitter.username"
Comment on attachment 8801339 [details]
MozReview: Review Page Redesign: Move commits table above the main content area (Bug 1309964).

https://reviewboard.mozilla.org/r/86118/#review88698

Did a quick sweep, didn't dive into the CSS much or all of the template stuff that was moved around. Some more screenshots would be helpful to allow commenting on different visual areas.

::: pylib/mozreview/mozreview/extension.py:318
(Diff revision 5)
>          ReviewRequestFieldsHook(self, 'main', [CommitsListField])
> +        MozReviewCommitTableHook(self, 'base-before-content',
> +                     'mozreview/commits.html',
> +                     apply_to=review_request_url_names)
> +
>          # This forces the Commits field to be the top item.
>          main_fieldset.field_classes.insert(0,
>                                             main_fieldset.field_classes.pop())

Couple of things here, the `MozReviewCommitTableHook` is a `TemplateHook` so it  would be better situated with all the other `TemplateHook`s above.

Additionally, inserting it here breaks up two statements which rely on being run in order, without other field insertion taking place. Adding the `CommitsListField` with a `ReviewRequestFieldsHook` is meant to be executed immediately before popping the `main_fieldset.field_classes` (which, is the `CommitsListField` we just added) and re-inserting as the first item. This requirement really should be explicitly documented here.

Though, since you remove the `CommitsListField`'s rendering it's not actually important to pop it and reinsert. Your new field should be done this way though if it's meant to come before the built-ins and be first. Since you're hiding everything else it might not be important, but we definitely don't want to do this with the `CommitListField` anymore. If you do use the same technique please add a comment documenting the hidden assumption I outlined.

::: pylib/mozreview/mozreview/hooks.py:5
(Diff revision 5)
>  from __future__ import unicode_literals
>  
>  import logging
>  
> -from reviewboard.extensions.hooks import ReviewRequestApprovalHook
> +from django.template.loader import Context, get_template

You don't actually use `get_template` in here, please give the python files a pyflakes / pep8 run and make sure you're not introducing new errors.

::: pylib/mozreview/mozreview/hooks.py:34
(Diff revision 5)
>  )
>  
>  
>  logger = logging.getLogger(__name__)
>  
> +class MozReviewCommitTableHook(TemplateHook):

This hook is actually more generic than just the commit table - It's a TemplateHook which can be used with any template hookpoint and template, but provides extra context our commit table template makes use of. I think a name like `CommitContextTemplateHook` makes more sense.

Alternatively, the `__init__` could be overriden to hardcode the template/hook point so that it's clear this isn't a generic template hook for MozReview that provides extra information.

::: pylib/mozreview/mozreview/hooks.py:34
(Diff revision 5)
> +class MozReviewCommitTableHook(TemplateHook):
> +
> +    def get_extra_context(self, request, context):

The class and its methods could use a nice docstring.

::: pylib/mozreview/mozreview/hooks.py:36
(Diff revision 5)
> +    def get_extra_context(self, request, context):
> +
> +        user = request.user
> +        review_request_details = context['review_request_details']
> +
> +        parent = get_parent_rr(review_request_details.get_review_request())
> +        parent_details = parent.get_draft(user) or parent
> +
> +        # If a user can view the parent draft they should also have
> +        # permission to view every child. We check if the child is
> +        # accessible anyways in case it has been restricted for other
> +        # reasons.
> +        children_details = [
> +            child for child in gen_child_rrs(parent_details, user=user)
> +            if child.is_accessible_by(user)]
> +
> +        try:
> +            current_child_index = children_details.index(review_request_details)
> +            current_child_num = current_child_index + 1
> +        except:
> +            current_child_num = current_child_index = None
> +            pass
> +
> +        next_child = prev_child = None
> +        if(current_child_index != None):
> +            if(current_child_index + 1 < len(children_details)):
> +                next_child = children_details[current_child_index + 1]
> +            if(current_child_index != 0):
> +                prev_child = children_details[current_child_index - 1]
> +
> +        autoland_requests = AutolandRequest.objects.filter(
> +            review_request_id=parent.id).order_by('-autoland_id')
> +
> +        repo_urls = set()
> +        latest_autoland_requests = []
> +
> +        commit_data = fetch_commit_data(review_request_details)
> +        commits_json = commit_data.get_for(review_request_details, 'p2rb.commits')
> +
> +        # We would like to fetch the latest AutolandRequest for each
> +        # different repository.
> +        for request in autoland_requests:
> +            if request.repository_url in repo_urls:
> +                continue
> +
> +            repo_urls.add(request.repository_url)
> +            latest_autoland_requests.append(request)
> +
> +        return {'review_request_details': review_request_details,
> +            'parent_details': parent_details,
> +            'children_details': children_details,
> +            'num_children': len(children_details),
> +            'current_child_num': current_child_num,
> +            'next_child': next_child,
> +            'prev_child': prev_child,
> +            'latest_autoland_requests': latest_autoland_requests,
> +            'user': user}

There's a few issues with this function, stylistically and functionally. As I found myself writing code suggestions for them all I concluded it was much easier to just suggest an alternative implementation of the body:
```Python
    def get_extra_context(self, request, context):
        review_request_details = context['review_request_details']
        commit_data = fetch_commit_data(review_request_details)

        user = request.user
        parent = get_parent_rr(review_request_details, commit_data=commit_data)
        parent_details = parent.get_draft(user) or parent

        # If a user can view the parent draft they should also have
        # permission to view every child. We check if the child is
        # accessible anyways in case it has been restricted for other
        # reasons.
        children_details = [
            child for child in gen_child_rrs(parent_details, user=user)
            if child.is_accessible_by(user)]
        n_children = len(children_details)
        current_child_num = prev_child = next_child = None

        if not is_parent(review_request_details, commit_data=commit_data):
            cur_index = children_details.index(review_request_details)
            current_child_num = cur_index - 1
            next_child = (children_details[cur_index + 1]
                          if cur_index + 1 < n_children else None)
            prev_child = (children_details[cur_index - 1]
                          if cur_index - 1 >= 0 else None)

        latest_autoland_requests = []
        repo_urls = set()
        autoland_requests = AutolandRequest.objects.filter(
            review_request_id=parent.id).order_by('-autoland_id')

        # We would like to fetch the latest AutolandRequest for each
        # different repository.
        for request in autoland_requests:
            if request.repository_url in repo_urls:
                continue

            repo_urls.add(request.repository_url)
            latest_autoland_requests.append(request)

        return {
            'review_request_details': review_request_details,
            'parent_details': parent_details,
            'children_details': children_details,
            'num_children': n_children,
            'current_child_num': current_child_num,
            'next_child': next_child,
            'prev_child': prev_child,
            'latest_autoland_requests': latest_autoland_requests,
            'user': user,
        }

```

::: pylib/mozreview/mozreview/templates/mozreview/commits-requests.html
(Diff revision 5)
>      <th class="submitter">{% trans "Submitter" %}</th>
>      {% endcomment %}
>      <th class="reviewers">{% trans "Reviewers" %}</th>
> -    <th class="status">
> -      {% trans "Landable" %}
> -      <div class="help-icon help-tooltip" title="{% trans "Hover over each commit's status to view landable status" %}"><span>?</span></div>

How are we planning to make the tooltips on the new status location discoverable?

::: pylib/mozreview/mozreview/templates/mozreview/commits.html:26
(Diff revision 5)
>    data-selected-review-id="{{ review_request_details.get_review_request.id }}"
>  ></div>
>  
>  <div id="mozreview-request-series">
> -  <div id="mozreview-parent-request">
> +  <div id="mozreview-review-header">
> +      {% if child_details.get_review_request.id = review_request_details.get_review_request.id %}

`==`, you've got a single `=` in this conditional. It would appear the previous code had these sprinkled through :(. Could you please do a sweep and check the conditionals, fixing this?

::: pylib/mozreview/mozreview/templates/mozreview/commits.html:39
(Diff revision 5)
> +       <a href="{{bug|bug_url:review_request_details}}">Bug #{{bug}}</a>{% if not forloop.last %}, {% endif %}
> +      {%    endfor %}
> +      &bull;
> +      {% endif %}
> +      {{parent_details.get_review_request.repository}}
> +      {% if parent_details.get_review_request.id = review_request_details.get_review_request.id %}

`=`
Attachment #8801339 - Flags: review?(smacleod) → review-
Comment on attachment 8801872 [details]
MozReview: Review Page Redesign: Update main content area contents (Bug 1309964).

https://reviewboard.mozilla.org/r/86476/#review88700

::: pylib/mozreview/mozreview/fields.py:102
(Diff revision 3)
> -        return False
> +        return is_pushed(self.review_request_details)
> +
> +    def as_html(self):
> +        user = self.request.user
> +        parent = get_parent_rr(self.review_request_details.get_review_request())
> +        parent_details = parent.get_draft(user) or parent
> +
> +        # If a user can view the parent draft they should also have
> +        # permission to view every child. We check if the child is
> +        # accessible anyways in case it has been restricted for other
> +        # reasons.
> +        children_details = [
> +            child for child in gen_child_rrs(parent_details, user=user)
> +            if child.is_accessible_by(user)]
> +
> +        return get_template('mozreview/review-main.html').render(Context({
> +            'review_request_details': self.review_request_details,
> +            'parent_details': parent_details,
> +            'user': user
> +        }))

rather than moving the field this is in later, please update this commit to take care of it.

::: pylib/mozreview/mozreview/fields.py:106
(Diff revision 3)
>      def should_render(self, value):
> -        return False
> +        return is_pushed(self.review_request_details)
> +
> +    def as_html(self):
> +        user = self.request.user
> +        parent = get_parent_rr(self.review_request_details.get_review_request())

no need to call get_review_request() for this.

::: pylib/mozreview/mozreview/static/mozreview/js/review.js:22
(Diff revision 3)
> +  // Move the position of the pull and import commit boxes
> +  $(document.getElementById('field_p2rb.PullCommitField')).find('input').appendTo('#review-request-inputs');
> +  $(document.getElementById('field_p2rb.ImportCommitField')).find('input').appendTo('#review-request-inputs');

These should really be integrated into the new field representing this box, rather than being fields themselves. It'll make pages load faster and not rely on javascript hackery here. Those fields are purely visual anyway so we don't need to keep them for data copying etc.

::: pylib/mozreview/mozreview/templates/mozreview/review-main.html:1
(Diff revision 3)
> +{% load djblets_utils i18n reviewtags staticfiles %}

Screenshots of all this stuff would be really helpful.

::: pylib/mozreview/mozreview/templates/mozreview/review-main.html:24
(Diff revision 3)
> +    r?
> +  </div>
> +{% endif %}
> +
> +<div class="review-request-meta">
> +  Authored by <a href="mailto:{{ review_request_details.get_review_request.submitter.email }}">{{ review_request_details.get_review_request.submitter.username }}</a>

This isn't technically true - this commit was "Submitted" or "Pushed" by the submitter, the actual commit author could be someone else. We also need to display the author (which is stored in the commit data) somewhere on this page.
Attachment #8801872 - Flags: review?(smacleod) → review-
Comment on attachment 8804899 [details]
MozReview: Review Page Redesign: Enhance table toggling and add cookie table state (Bug 1309964).

https://reviewboard.mozilla.org/r/88728/#review88702

Nothing glaring with a quick glance, I'll let glob dig in here more.
Attachment #8804899 - Flags: review?(smacleod)
Comment on attachment 8804900 [details]
MozReview: Review Page Redesign: Ensure draft banner displays at top of page (Bug 1309964).

https://reviewboard.mozilla.org/r/88730/#review88704

::: pylib/mozreview/mozreview/static/mozreview/js/review.js:47
(Diff revision 2)
> +  // Ensure the draft banner displays in the correct place (above the commits table)
> +  $('#review_request_banners').insertBefore($('#error-container'));

We should fix this the way I noted in the first commit, rather than through javascript. Let me know if you need a more detailed explanation.
Attachment #8804900 - Flags: review?(smacleod) → review-
Comment on attachment 8804901 [details]
MozReview: Review Page Redesign: Temprorarily hide the review status from the main content area (Bug 1309964).

https://reviewboard.mozilla.org/r/88732/#review88706

::: pylib/mozreview/mozreview/templates/mozreview/review-main.html:7
(Diff revision 2)
> +{% comment "TODO: Temporarily remove this until we can 'live update' it like the commits table when an issue is closed" %}
>  {% if review_request_details.get_review_request.issue_open_count > 0 %}
>    <div class="status approval-issues" title="{{review_request_details.get_review_request.issue_open_count}} open issues">
>      <a class="issue-count" href="{{review_request_details.get_review_request.get_absolute_url}}#issue-summary">
>        ! {{review_request_details.get_review_request.issue_open_count}}
>      </a>
>    </div>
>  {% elif review_request_details.get_review_request.approved %}
>    <div class="status approval" title="Approved For Landing - You have at least one valid ship it!">
>      r+
>    </div>
>  {% else %}
>    <div class="status no-approval" title="{{review_request_details.get_review_request.approval_failure}}">
>      r?
>    </div>
>  {% endif %}
> +{% endcomment %}

Errr... I don't think I agree with this. We need to either leave as is, or actually fix the live updating before landing this stuff.
Attachment #8804901 - Flags: review?(smacleod) → review-
Comment on attachment 8805184 [details]
MozReview: Review Page Redesign: Created new 'ReviewContentField' class for review main content (Bug 1309964).

https://reviewboard.mozilla.org/r/88980/#review88708

I'll give another review of the change that deals with this field when it's folded.

::: pylib/mozreview/mozreview/extension.py:55
(Diff revision 1)
>      CommitAuthorField,
>      CommitsListField,
>      FileDiffReviewerField,
>      ImportCommitField,
>      PullCommitField,
> +    ReviewContentField,

Ya, this should really be folded into the commit which adds the rendering and template code.
Attachment #8805184 - Flags: review?(smacleod) → review-
Comment on attachment 8805176 [details]
MozReview: Review Page Redesign: Add user session cookie for commits table persistence (Bug 1309964).

https://reviewboard.mozilla.org/r/88976/#review88710

Going to leave this to glob, he worked with this stuff recently. If there is a way we can do this in the extension instead of the fork that'd be preferable, we're kinda breaking the division with this change.
Attachment #8805176 - Flags: review?(smacleod)
Comment on attachment 8805176 [details]
MozReview: Review Page Redesign: Add user session cookie for commits table persistence (Bug 1309964).

https://reviewboard.mozilla.org/r/88976/#review88804
Attachment #8805176 - Flags: review?(glob) → review+
Comment on attachment 8801339 [details]
MozReview: Review Page Redesign: Move commits table above the main content area (Bug 1309964).

https://reviewboard.mozilla.org/r/86118/#review88794

as smacleod pointed out this is hard to review as there are multiple edits to the same file split across revisions; it would be better to fold later changes into this first revision.

::: pylib/mozreview/mozreview/static/mozreview/css/commits.less:22
(Diff revision 5)
>  
>  @import (reference) 'mozilla-theme-common.less';
>  
>  @approval-bg: #DFFFD7;
>  @approval-border-color: #478A06;
> +@table-large-font: 18px;

for clarity this should be named @table-large-font-size

::: pylib/mozreview/mozreview/static/mozreview/css/mozilla-theme-reviews.less:25
(Diff revision 5)
> +  .header, label[for="field_p2rb.commits"] {
> +    display: none;
> +  }

would it be better to not render this field instead of just hiding it?

::: pylib/mozreview/mozreview/static/mozreview/css/mozilla-theme.less:5
(Diff revision 5)
>  @import (reference) 'mozilla-theme-common.less';
>  
>  body {
>    background: @moz-blue-light;
> -  font-size: 13px;
> +  font-family: arial;

arial?  :)

if desired you can use almost any of the google web fonts.

https://fonts.google.com/attribution shows most typefaces are covered by OFL which allows for redistribution (http://scripts.sil.org/cms/scripts/page.php?item_id=OFL-FAQ_web)

::: pylib/mozreview/mozreview/templates/mozreview/commits-requests.html:23
(Diff revision 5)
> +      <td colspan="2">
> +        {% if prev_child %}
> +          <a href="{{prev_child.get_review_request.get_absolute_url}}" rel="previous">&lt;&lt; Previous Commit</a>
> +        {% endif %}
> +        Viewing commit {{current_child_num}} of {{num_children}}. <a href="javascript:;" id="mozreview-all-commits">View All Commits</a>
> +        {% if next_child %}
> +          <a href="{{next_child.get_review_request.get_absolute_url}}" rel="next">Next Commit &gt;&gt;</a>
> +        {% endif %}
> +      </td>

incorrect indentation

::: pylib/mozreview/mozreview/templates/mozreview/commits.html:34
(Diff revision 5)
> -    <a href="{{parent_details.get_review_request.get_absolute_url}}">Review Summary</a>
> +        <a href="{{parent_details.get_review_request.get_absolute_url}}">Review Summary</a>
> -    <a href="{{parent_details.get_review_request.get_absolute_url}}diff/#index_header">Squashed Diff</a>
> +      {% endif %}
> +      &bull;
> +      {% if review_request_details.get_review_request.bugs_closed %}
> +      {%    for bug in review_request_details.get_bug_list %}
> +       <a href="{{bug|bug_url:review_request_details}}">Bug #{{bug}}</a>{% if not forloop.last %}, {% endif %}

nit: weird indentation

::: pylib/mozreview/mozreview/templates/mozreview/commits.html:40
(Diff revision 5)
> +      {%    endfor %}
> +      &bull;
> +      {% endif %}
> +      {{parent_details.get_review_request.repository}}
> +      {% if parent_details.get_review_request.id = review_request_details.get_review_request.id %}
> +        &bull; <a href="{{parent_details.get_review_request.get_absolute_url}}diff/#index_header">Squashed Diff</a>

nit: indentation
Attachment #8801339 - Flags: review?(glob) → review-
Comment on attachment 8801872 [details]
MozReview: Review Page Redesign: Update main content area contents (Bug 1309964).

https://reviewboard.mozilla.org/r/86476/#review88798

::: pylib/mozreview/mozreview/static/mozreview/css/mozilla-theme.less:263
(Diff revision 3)
> +  > div:nth-of-type(2),
> +  > div:nth-of-type(3) {

using nth-of-type is a potential footgun - use class or ids to select please
Attachment #8801872 - Flags: review?(glob) → review-
Comment on attachment 8804899 [details]
MozReview: Review Page Redesign: Enhance table toggling and add cookie table state (Bug 1309964).

https://reviewboard.mozilla.org/r/88728/#review88790

::: pylib/mozreview/mozreview/static/mozreview/css/mozilla-theme-reviews.less:105
(Diff revision 2)
>  }
>  
>  .status {
>    .review-status-icon();
>    font-size: 20px;
> -  margin-bottom: 10px;
> +  /* margin-bottom: 10px; */

remove commented out code

::: pylib/mozreview/mozreview/static/mozreview/js/review.js:43
(Diff revision 2)
> +  $('#mozreview-commits-presist input').on('change', function(e) {
> +    RB.UserSession.instance.set('commitsTableAlwaysShowFull', this.checked);
>    });

you need to initialise the collapsed state of the table at page load time if the cookie is true

::: pylib/mozreview/mozreview/templates/mozreview/commits-requests.html:28
(Diff revision 2)
>        <td colspan="2">
>          {% if prev_child %}
>            <a href="{{prev_child.get_review_request.get_absolute_url}}" rel="previous">&lt;&lt; Previous Commit</a>
>          {% endif %}
> -        Viewing commit {{current_child_num}} of {{num_children}}. <a href="javascript:;" id="mozreview-all-commits">View All Commits</a>
> +        Viewing commit {{current_child_num}} of {{num_children}}.
> +        <a href="javascript:;" data-one-text="View This Commit" data-all-text="View All Commits" id="mozreview-all-commits">{% if request.COOKIES.commits_table_show == 'true' %}View This Commit{% else %}View All Commits{% endif %}</a>

use RB.UserSession.instance.get('commitsTableAlwaysShowFull') instead of reading the cookie directly

::: pylib/mozreview/mozreview/templates/mozreview/commits-requests.html:33
(Diff revision 2)
> +        <a href="javascript:;" data-one-text="View This Commit" data-all-text="View All Commits" id="mozreview-all-commits">{% if request.COOKIES.commits_table_show == 'true' %}View This Commit{% else %}View All Commits{% endif %}</a>
>          {% if next_child %}
>            <a href="{{next_child.get_review_request.get_absolute_url}}" rel="next">Next Commit &gt;&gt;</a>
>          {% endif %}
> +        <br />
> +        <label id="mozreview-commits-presist"><input type="checkbox" {% if request.COOKIES.commits_table_show == 'true' %}checked{% endif %}> Keep table expanded</label>

not sure about this - would it be better to remember the expanded/collapsed state when it's toggle?
Attachment #8804899 - Flags: review?(glob) → review-
Comment on attachment 8804900 [details]
MozReview: Review Page Redesign: Ensure draft banner displays at top of page (Bug 1309964).

https://reviewboard.mozilla.org/r/88730/#review88806
Attachment #8804900 - Flags: review?(glob)
Comment on attachment 8804901 [details]
MozReview: Review Page Redesign: Temprorarily hide the review status from the main content area (Bug 1309964).

https://reviewboard.mozilla.org/r/88732/#review88808
Attachment #8804901 - Flags: review?(glob)
Comment on attachment 8805184 [details]
MozReview: Review Page Redesign: Created new 'ReviewContentField' class for review main content (Bug 1309964).

https://reviewboard.mozilla.org/r/88980/#review88810
Attachment #8805184 - Flags: review?(glob)
Comment on attachment 8801339 [details]
MozReview: Review Page Redesign: Move commits table above the main content area (Bug 1309964).

https://reviewboard.mozilla.org/r/86118/#review88794

> would it be better to not render this field instead of just hiding it?

Is there a way to hide the header/label?  If you don't provide one, django outputs `None`
Comment on attachment 8804899 [details]
MozReview: Review Page Redesign: Enhance table toggling and add cookie table state (Bug 1309964).

https://reviewboard.mozilla.org/r/88728/#review88790

> use RB.UserSession.instance.get('commitsTableAlwaysShowFull') instead of reading the cookie directly

There are two ways to do this, I guess:

1. Using `request.COOKIES.commits_table_show` on the server side to set the initial closed/open state, which prevents a flash of content issue

2. Doing *everything* with JavaScript but that could lead to FOC
Comment on attachment 8801339 [details]
MozReview: Review Page Redesign: Move commits table above the main content area (Bug 1309964).

https://reviewboard.mozilla.org/r/86118/#review88794

> Is there a way to hide the header/label?  If you don't provide one, django outputs `None`

If you have `should_render()` returning `False`, the label itself shouldn't even be rendered... Are you sure it still is or might this just be a holdover before you fiddled with the field?
Comment on attachment 8804901 [details]
MozReview: Review Page Redesign: Temprorarily hide the review status from the main content area (Bug 1309964).

https://reviewboard.mozilla.org/r/88732/#review88706

> Errr... I don't think I agree with this. We need to either leave as is, or actually fix the live updating before landing this stuff.

This becomes complicated because @glob simply refreshes the contents of the table; I guess we could parse out the `r` state but I beleive @mcote mentioned it was required for this first run.
Comment on attachment 8804900 [details]
MozReview: Review Page Redesign: Ensure draft banner displays at top of page (Bug 1309964).

https://reviewboard.mozilla.org/r/88730/#review88704

> We should fix this the way I noted in the first commit, rather than through javascript. Let me know if you need a more detailed explanation.

What's the best route for taking these items from fields.py (`ImportCommitField` and `PullCommitField`) and simply outputting them in the template?  Where should all of that logic go?
Comment on attachment 8804901 [details]
MozReview: Review Page Redesign: Temprorarily hide the review status from the main content area (Bug 1309964).

https://reviewboard.mozilla.org/r/88732/#review88706

> This becomes complicated because @glob simply refreshes the contents of the table; I guess we could parse out the `r` state but I beleive @mcote mentioned it was required for this first run.

Also remember that the status still displays in the table at the top.  I would agree that it's important to have the status repeated below.  Any idea for how to get that information or should we try to parse the table?
Comment on attachment 8804900 [details]
MozReview: Review Page Redesign: Ensure draft banner displays at top of page (Bug 1309964).

https://reviewboard.mozilla.org/r/88730/#review88704

> What's the best route for taking these items from fields.py (`ImportCommitField` and `PullCommitField`) and simply outputting them in the template?  Where should all of that logic go?

Grr, commented on the wrong RR.  Ignore my comment.
Comment on attachment 8801872 [details]
MozReview: Review Page Redesign: Update main content area contents (Bug 1309964).

https://reviewboard.mozilla.org/r/86476/#review88700

> These should really be integrated into the new field representing this box, rather than being fields themselves. It'll make pages load faster and not rely on javascript hackery here. Those fields are purely visual anyway so we don't need to keep them for data copying etc.

What's the best route for taking these items from fields.py (`ImportCommitField` and `PullCommitField`) and simply outputting them in the template?  Where should all of that logic go?
Comment on attachment 8801339 [details]
MozReview: Review Page Redesign: Move commits table above the main content area (Bug 1309964).

https://reviewboard.mozilla.org/r/86118/#review88698

> There's a few issues with this function, stylistically and functionally. As I found myself writing code suggestions for them all I concluded it was much easier to just suggest an alternative implementation of the body:
> ```Python
>     def get_extra_context(self, request, context):
>         review_request_details = context['review_request_details']
>         commit_data = fetch_commit_data(review_request_details)
> 
>         user = request.user
>         parent = get_parent_rr(review_request_details, commit_data=commit_data)
>         parent_details = parent.get_draft(user) or parent
> 
>         # If a user can view the parent draft they should also have
>         # permission to view every child. We check if the child is
>         # accessible anyways in case it has been restricted for other
>         # reasons.
>         children_details = [
>             child for child in gen_child_rrs(parent_details, user=user)
>             if child.is_accessible_by(user)]
>         n_children = len(children_details)
>         current_child_num = prev_child = next_child = None
> 
>         if not is_parent(review_request_details, commit_data=commit_data):
>             cur_index = children_details.index(review_request_details)
>             current_child_num = cur_index - 1
>             next_child = (children_details[cur_index + 1]
>                           if cur_index + 1 < n_children else None)
>             prev_child = (children_details[cur_index - 1]
>                           if cur_index - 1 >= 0 else None)
> 
>         latest_autoland_requests = []
>         repo_urls = set()
>         autoland_requests = AutolandRequest.objects.filter(
>             review_request_id=parent.id).order_by('-autoland_id')
> 
>         # We would like to fetch the latest AutolandRequest for each
>         # different repository.
>         for request in autoland_requests:
>             if request.repository_url in repo_urls:
>                 continue
> 
>             repo_urls.add(request.repository_url)
>             latest_autoland_requests.append(request)
> 
>         return {
>             'review_request_details': review_request_details,
>             'parent_details': parent_details,
>             'children_details': children_details,
>             'num_children': n_children,
>             'current_child_num': current_child_num,
>             'next_child': next_child,
>             'prev_child': prev_child,
>             'latest_autoland_requests': latest_autoland_requests,
>             'user': user,
>         }
> 
> ```

With this code `current_child_num` is `0` or `-1` -- something's off there.
Comment on attachment 8801339 [details]
MozReview: Review Page Redesign: Move commits table above the main content area (Bug 1309964).

https://reviewboard.mozilla.org/r/86118/#review88698

> With this code `current_child_num` is `0` or `-1` -- something's off there.

Bah, sorry, `current_child_num = cur_index - 1` is incorrect, it should be `current_child_num = cur_index + 1`
Comment on attachment 8801339 [details]
MozReview: Review Page Redesign: Move commits table above the main content area (Bug 1309964).

https://reviewboard.mozilla.org/r/86118/#review88698

> Couple of things here, the `MozReviewCommitTableHook` is a `TemplateHook` so it  would be better situated with all the other `TemplateHook`s above.
> 
> Additionally, inserting it here breaks up two statements which rely on being run in order, without other field insertion taking place. Adding the `CommitsListField` with a `ReviewRequestFieldsHook` is meant to be executed immediately before popping the `main_fieldset.field_classes` (which, is the `CommitsListField` we just added) and re-inserting as the first item. This requirement really should be explicitly documented here.
> 
> Though, since you remove the `CommitsListField`'s rendering it's not actually important to pop it and reinsert. Your new field should be done this way though if it's meant to come before the built-ins and be first. Since you're hiding everything else it might not be important, but we definitely don't want to do this with the `CommitListField` anymore. If you do use the same technique please add a comment documenting the hidden assumption I outlined.

I don't think I'm understanding you here.  I've tried removing these lines:

```
main_fieldset.field_classes.insert(0, main_fieldset.field_classes.pop())
```

... but the page doesn't render properly -- the old commits content displays there.

What am I missing?
Comment on attachment 8801339 [details]
MozReview: Review Page Redesign: Move commits table above the main content area (Bug 1309964).

https://reviewboard.mozilla.org/r/86118/#review88698

> I don't think I'm understanding you here.  I've tried removing these lines:
> 
> ```
> main_fieldset.field_classes.insert(0, main_fieldset.field_classes.pop())
> ```
> 
> ... but the page doesn't render properly -- the old commits content displays there.
> 
> What am I missing?

What do you mean by "the old commits content"? removing the pop and insert just means we're no longer doing positioning shenanigans with the CommitList rendering, you still need to make it not render so it doesn't show up.
Comment on attachment 8801872 [details]
MozReview: Review Page Redesign: Update main content area contents (Bug 1309964).

https://reviewboard.mozilla.org/r/86476/#review88700

> What's the best route for taking these items from fields.py (`ImportCommitField` and `PullCommitField`) and simply outputting them in the template?  Where should all of that logic go?

The logic for generating the context should be consolidated (they do a lot of the same thing) and integrated into your new field which is used for the main review request information area. They'll just become part of that template.
Comment on attachment 8804899 [details]
MozReview: Review Page Redesign: Enhance table toggling and add cookie table state (Bug 1309964).

https://reviewboard.mozilla.org/r/88728/#review88790

> There are two ways to do this, I guess:
> 
> 1. Using `request.COOKIES.commits_table_show` on the server side to set the initial closed/open state, which prevents a flash of content issue
> 
> 2. Doing *everything* with JavaScript but that could lead to FOC

I'm in favor of doing the initial load correctly server-side.
Comment on attachment 8804901 [details]
MozReview: Review Page Redesign: Temprorarily hide the review status from the main content area (Bug 1309964).

https://reviewboard.mozilla.org/r/88732/#review88706

> Also remember that the status still displays in the table at the top.  I would agree that it's important to have the status repeated below.  Any idea for how to get that information or should we try to parse the table?

TBH, the proper way is probably to switch the commit status updates, and updates of this, to a proper API call with JS rendering. I was totally spacing that you still have this in the table, I think I'm fine with that for now.

That being said, a follow up to integrate this stuff down here makes a lot of sense - especially since it gives up a nice place to really expand on the current status information with more than a tooltip. If you're not going to handle it here, please file a bug and drop this issue.
Comment on attachment 8804901 [details]
MozReview: Review Page Redesign: Temprorarily hide the review status from the main content area (Bug 1309964).

https://reviewboard.mozilla.org/r/88732/#review88706

> TBH, the proper way is probably to switch the commit status updates, and updates of this, to a proper API call with JS rendering. I was totally spacing that you still have this in the table, I think I'm fine with that for now.
> 
> That being said, a follow up to integrate this stuff down here makes a lot of sense - especially since it gives up a nice place to really expand on the current status information with more than a tooltip. If you're not going to handle it here, please file a bug and drop this issue.

Marking as fixed due to "That being said, a follow up to integrate this stuff down here makes a lot of sense"
Comment on attachment 8801872 [details]
MozReview: Review Page Redesign: Update main content area contents (Bug 1309964).

https://reviewboard.mozilla.org/r/86476/#review88798

> using nth-of-type is a potential footgun - use class or ids to select please

Note to self:  try `:empty` instead!
Comment on attachment 8804900 [details]
MozReview: Review Page Redesign: Ensure draft banner displays at top of page (Bug 1309964).

https://reviewboard.mozilla.org/r/88730/#review88704

> Grr, commented on the wrong RR.  Ignore my comment.

Dropping this issue because I'm going to strip this commit and perform the fix in the first commit, where it belongs.
Attachment #8804900 - Attachment is obsolete: true
Attachment #8805184 - Attachment is obsolete: true
Comment on attachment 8801872 [details]
MozReview: Review Page Redesign: Update main content area contents (Bug 1309964).

https://reviewboard.mozilla.org/r/86476/#review88700

> Screenshots of all this stuff would be really helpful.

Will add screenshots shortly, resetting so issue doesn't look outstanding.
Depends on: 1314430
Comment on attachment 8804901 [details]
MozReview: Review Page Redesign: Temprorarily hide the review status from the main content area (Bug 1309964).

https://reviewboard.mozilla.org/r/88732/#review89404

Bug files here:  https://bugzilla.mozilla.org/show_bug.cgi?id=1314430
david - looks like the changes to reviewboard have a pull of my inline comments patch as their parent instead of the latest public tip:

reviews/vct/reviewboard-fork$ wip
o  75 tip MozReview: Review Page Redesign: Add 'mozreview-review-content' template hook (Bug 1309964) r?glob,smacleod
o  74 MozReview: Review Page Redesign: Add review templates for future 'mozreview-review-content' modification (Bug 1309964). r?glob,smacleod
o  73 MozReview: Review Page Redesign: Add user session cookie for commits table persistence (Bug 1309964). r?glob,smacleod
o  72 Add toggle for inline comment visibility (bug 1115707) r?smacleod
o  71 clone for mozreview modification (bug 1115707); r?smacleod
o  70 enable inline comments via admin setting (bug 1115707) r?smacleod
o  69 clone for mozreview modification (bug 1115707); r?smacleod
o  68 add inline-comments admin setting (bug 1115707); r?smacleod
o  67 mozreview: add inline-comments js, html, css (bug 1115707); r?davidwalsh,smacleod
o  66 clone for mozreview modification (bug 1115707); r?smacleod
o  65 use querySelector instead of hand-rolled code (bug 1115707); r?smacleod
o  64 clone for mozreview modification (bug 1115707); r?smacleod
o  63 Don't ignore .orig or .rej files (bug 1115707); r?smacleod
o  62 mozreview: don't compress javascript inside the dev environment (bug 1115707); r?smacleod
| @  61 @ MozReview: Provide close button for dropdown dialog (Bug 1308291). r=glob
|/
o  34 @default Show confirmation when creating a new comment (bug 1275865) r=davidwalsh
|
~

i've rebased locally and can continue my review, but it'll need to be rebased correctly before it can be landed.
Comment on attachment 8806468 [details]
MozReview: Review Page Redesign: Add review templates for future 'mozreview-review-content' modification (Bug 1309964).

https://reviewboard.mozilla.org/r/89892/#review90696
Attachment #8806468 - Flags: review?(glob) → review+
Attachment #8806469 - Attachment is obsolete: true
Attachment #8806469 - Flags: review?(smacleod)
Attachment #8806469 - Flags: review?(glob)
Comment on attachment 8808245 [details]
MozReview: Review Page Redesign: Add 'mozreview-review-content' template hook (Bug 1309964)

https://reviewboard.mozilla.org/r/91084/#review90858

Rebased!
Blocks: 1314430
No longer depends on: 1314430
Comment on attachment 8801339 [details]
MozReview: Review Page Redesign: Move commits table above the main content area (Bug 1309964).

https://reviewboard.mozilla.org/r/86118/#review90704

::: pylib/mozreview/mozreview/static/mozreview/css/mozilla-theme.less:1
(Diff revision 6)
>  @import (reference) 'mozilla-theme-common.less';

not sure what's causing it, so attaching to a random line.

there's a line under the currently selected tab:

https://www.dropbox.com/s/3lrkmab4gg6qxn1/Screenshot%202016-11-07%2012.45.25.png?dl=0

::: pylib/mozreview/mozreview/static/mozreview/css/review.less:209
(Diff revision 6)
> -  top: -6px;
> -  right: 90px;
> +  top: 10px;
> +  right: 20px;

please change the position of the tooltip.

quite often i went to click on a commit's description to switch to it, only for the tooltip to obstruct my click.

https://gfycat.com/ShortEducatedArcticwolf
Attachment #8801339 - Flags: review?(glob) → review-
Comment on attachment 8801872 [details]
MozReview: Review Page Redesign: Update main content area contents (Bug 1309964).

https://reviewboard.mozilla.org/r/86476/#review90702

::: pylib/mozreview/mozreview/static/mozreview/css/mozilla-theme-common.less:1
(Diff revision 4)
> +@approval-bg: #DFFFD7;

nit: should be approval-background-color (mostly concerned about the -color suffix)

::: pylib/mozreview/mozreview/static/mozreview/css/mozilla-theme.less:259
(Diff revision 4)
>  .review-request #review_request_details {
> -  border-left: 1px solid @moz-blue-light;
> -  padding: 0 10px;
> +  display: none;
> +}
> +
> +.review-request #review_request_main {
> +  height: auto !important; /* Uses !important to block JavaScript from changing height */

would it be possible to instead update the js so it doesn't try to change the height?  that would remove some of the jank you get when resizing a window.
Attachment #8801872 - Flags: review?(glob) → review-
Comment on attachment 8804899 [details]
MozReview: Review Page Redesign: Enhance table toggling and add cookie table state (Bug 1309964).

https://reviewboard.mozilla.org/r/88728/#review90700

::: pylib/mozreview/mozreview/templates/mozreview/commits-requests.html:33
(Diff revision 3)
> -      <a href="javascript:;" id="mozreview-all-commits">View All Commits</a>
> +      <a href="javascript:;" data-one-text="View This Commit" data-all-text="View All Commits" id="mozreview-all-commits">{% if request.COOKIES.commits_table_show == 'true' %}View This Commit{% else %}View All Commits{% endif %}</a>
>        {% if next_child %}
>          <a href="{{next_child.get_review_request.get_absolute_url}}" rel="next">Next Commit &gt;&gt;</a>
>        {% endif %}
> +      <br />
> +      <label id="mozreview-commits-presist"><input type="checkbox" {% if request.COOKIES.commits_table_show == 'true' %}checked{% endif %}> Keep table expanded</label>

"Keep table expanded" while technically correct, isn't descriptive, especially since the checkbox doesn't appear do anything when the page is initially loaded.

i suggest:
- changing the text to "Always show all commits"
- when checked, immediately show all commits
Attachment #8804899 - Flags: review?(glob) → review-
Comment on attachment 8808245 [details]
MozReview: Review Page Redesign: Add 'mozreview-review-content' template hook (Bug 1309964)

https://reviewboard.mozilla.org/r/91084/#review91598

::: reviewboard/reviewboard/templates/diffviewer/view_diff_mozreview.html:39
(Diff revision 1)
>  
>  <div id="review_request">
>   <div id="review_request_banners"></div>
>  {% display_review_request_trophies review_request %}
>  
> +{% template_hook_point "mozreview-review-content" %}

I feel like "mozreview-pre-review-request-box" is a much more informative name for this.
Attachment #8808245 - Flags: review+
Comment on attachment 8804899 [details]
MozReview: Review Page Redesign: Enhance table toggling and add cookie table state (Bug 1309964).

https://reviewboard.mozilla.org/r/88728/#review90700

> "Keep table expanded" while technically correct, isn't descriptive, especially since the checkbox doesn't appear do anything when the page is initially loaded.
> 
> i suggest:
> - changing the text to "Always show all commits"
> - when checked, immediately show all commits

I've updated the language as requested, but line 10 should ensure all table items are expanded if the cookie is `true`.  My dev environment is hosed so please let me know if that's working.
Comment on attachment 8801339 [details]
MozReview: Review Page Redesign: Move commits table above the main content area (Bug 1309964).

https://reviewboard.mozilla.org/r/86118/#review91600

LGTM based on the code - I'm leaving the testing and final review decision to glob.

::: pylib/mozreview/mozreview/extension.py:319
(Diff revision 9)
> -        # This forces the Commits field to be the top item.
> -        main_fieldset.field_classes.insert(0,
> -                                           main_fieldset.field_classes.pop())
> +        CommitContextTemplateHook(self, 'mozreview-review-content',
> +                     'mozreview/commits.html',
> +                     apply_to=review_request_url_names)

argument indentation is off. should be aligned with
same line arguments, or indented 4:

```
CommitContextTemplateHook(self, 'mozreview-review-content',
                          'mozreview/commits.html',
                          apply_to=review_request_url_names)
```

or

```
CommitContextTemplateHook(self, 'mozreview-review-content',
    'mozreview/commits.html', apply_to=review_request_url_names)
```

::: pylib/mozreview/mozreview/hooks.py:43
(Diff revision 9)
> +    Information provided includes the parent and child review requests,
> +    as well as autoland information.
> +    """
> +
> +    def get_extra_context(self, request, context):
> +        """Fetches relevant review request information, returns content"""

s/content/context
Attachment #8801339 - Flags: review?(smacleod) → review+
Comment on attachment 8801872 [details]
MozReview: Review Page Redesign: Update main content area contents (Bug 1309964).

https://reviewboard.mozilla.org/r/86476/#review91746

::: pylib/mozreview/mozreview/fields.py:128
(Diff revision 7)
> -class ImportCommitField(BaseReviewRequestField):
> -    """This field provides some information on how to pull down the commit"""
> +class ReviewContentField(BaseReviewRequestField):
> +    """This field provides the main content for reviews"""

a "review" is a very specific type of object in RB, "review" != "review request".

We should probably just name this around it being a specific commit, like you use for the `field_id`.

::: pylib/mozreview/mozreview/fields.py:139
(Diff revision 7)
> -        return not is_parent(self.review_request_details, self.commit_data)
> +        return is_pushed(self.review_request_details)
>  
>      def as_html(self):
> -        commit_id = self.commit_data.extra_data.get(COMMIT_ID_KEY)
> -        review_request = self.review_request_details.get_review_request()
> -        repo_path = review_request.repository.path
> +        user = self.request.user
> +        parent = get_parent_rr(self.review_request_details)
> +        parent_details = parent.get_draft(user) or parent

If `self.review_request_details` is a draft already, then `parent` will also be the draft, which will cause `parent.get_draft(user)` to fail because drafts do not have a `get_draft` method.

you could solve this by making sure you don't have the draft above with `get_review_request()`

::: pylib/mozreview/mozreview/fields.py:162
(Diff revision 7)
> +            'pull_text': self.get_pull_text(),
> +            'import_text': self.get_import_text(),
> +        }))
>  
> -    def as_html(self):
> -        commit_id = self.commit_data.extra_data.get(COMMIT_ID_KEY)
> +    def get_pull_text(self):
> +        commit_data = fetch_commit_data(self.review_request_details)

Rather than fetching this several times, you should just do what the old field did and save it as an attribute of the object during `__init__`

::: pylib/mozreview/mozreview/fields.py:171
(Diff revision 7)
>              children = [
>                  child for child in gen_child_rrs(parent_details, user=user)
>                  if child.is_accessible_by(user)]

You're doing this already as part of the main render. You should really consolidate all of the fetching of this data.

::: pylib/mozreview/mozreview/fields.py:188
(Diff revision 7)
>                  review_request.id))
>              return ''
>  
> -        return get_template('mozreview/hg-pull.html').render(Context({
> -                'commit_id': commit_id,
> -                'repo_path': repo_path,
> +        return "hg pull -r %s %s" % (commit_id, repo_path)
> +
> +    def get_import_text(self):

Ya, this shares a lot of data with the pull command. Please consolidate it all into the render method, or pass the needed data in at least, so we're not needlessly hitting the DB / repeating a bunch of code.

::: pylib/mozreview/mozreview/static/mozreview/js/review.js:22
(Diff revision 7)
> +  // Move the position of the pull and import commit boxes
> +  $(document.getElementById('field_p2rb.PullCommitField')).find('input').appendTo('#review-request-inputs');
> +  $(document.getElementById('field_p2rb.ImportCommitField')).find('input').appendTo('#review-request-inputs');

This should no longer be required, these elements should be rendered as part of the new field.
Attachment #8801872 - Flags: review?(smacleod) → review-
Comment on attachment 8801872 [details]
MozReview: Review Page Redesign: Update main content area contents (Bug 1309964).

https://reviewboard.mozilla.org/r/86476/#review91746

> This should no longer be required, these elements should be rendered as part of the new field.

Ugh, this probably got put back during my massive histedit conflicts.  Sorry.
Comment on attachment 8804899 [details]
MozReview: Review Page Redesign: Enhance table toggling and add cookie table state (Bug 1309964).

https://reviewboard.mozilla.org/r/88728/#review91790
Attachment #8804899 - Flags: review?(smacleod)
Comment on attachment 8801872 [details]
MozReview: Review Page Redesign: Update main content area contents (Bug 1309964).

https://reviewboard.mozilla.org/r/86476/#review91792

::: pylib/mozreview/mozreview/templates/mozreview/review-main.html:1
(Diff revision 7)
> +{% load djblets_utils i18n reviewtags staticfiles %}

Forgot to say, this file shouldn't be called "review-main", review is distinct from review request.
Comment on attachment 8804901 [details]
MozReview: Review Page Redesign: Temprorarily hide the review status from the main content area (Bug 1309964).

https://reviewboard.mozilla.org/r/88732/#review91794

::: pylib/mozreview/mozreview/templates/mozreview/review-main.html:7
(Diff revision 6)
> +{% comment "TODO: Temporarily remove this until we can 'live update' it like the commits table when an issue is closed" %}
>  {% if review_request_details.get_review_request.issue_open_count > 0 %}
>    <div class="status approval-issues" title="{{review_request_details.get_review_request.issue_open_count}} open issues">
>      <a class="issue-count" href="{{review_request_details.get_review_request.get_absolute_url}}#issue-summary">
>        ! {{review_request_details.get_review_request.issue_open_count}}
>      </a>
>    </div>
>  {% elif review_request_details.get_review_request.approved %}
>    <div class="status approval" title="Approved For Landing - You have at least one valid ship it!">
>      r+
>    </div>
>  {% else %}
>    <div class="status no-approval" title="{{review_request_details.get_review_request.approval_failure}}">
>      r?
>    </div>
>  {% endif %}
> +{% endcomment %}

I think it's probably fine to just nuke this section - we can always ressurect it from version control.
Attachment #8804901 - Flags: review?(smacleod) → review+
Comment on attachment 8804899 [details]
MozReview: Review Page Redesign: Enhance table toggling and add cookie table state (Bug 1309964).

https://reviewboard.mozilla.org/r/88728/#review90700

> I've updated the language as requested, but line 10 should ensure all table items are expanded if the cookie is `true`.  My dev environment is hosed so please let me know if that's working.

sorry my description wasn't clear enough.
the cookie works, but only on page load.  

str
- ensure the cookie is either not set or is false
- load the page (which results in a single commit being visible)
- check the checkbox

expected:
- all commits should immediately be visible

actual:
- nothing; a page refresh is required for the checkbox to take effect
Comment on attachment 8804899 [details]
MozReview: Review Page Redesign: Enhance table toggling and add cookie table state (Bug 1309964).

https://reviewboard.mozilla.org/r/88728/#review90700

> sorry my description wasn't clear enough.
> the cookie works, but only on page load.  
> 
> str
> - ensure the cookie is either not set or is false
> - load the page (which results in a single commit being visible)
> - check the checkbox
> 
> expected:
> - all commits should immediately be visible
> 
> actual:
> - nothing; a page refresh is required for the checkbox to take effect

I've implemented it as you described; I do not, however, collapse the table if the toggle is unchecked.  The checkbox is function-separate from the link.
Attachment #8804901 - Attachment is obsolete: true
Attachment #8804901 - Flags: review?(glob)
Comment on attachment 8801872 [details]
MozReview: Review Page Redesign: Update main content area contents (Bug 1309964).

https://reviewboard.mozilla.org/r/86476/#review91746

> If `self.review_request_details` is a draft already, then `parent` will also be the draft, which will cause `parent.get_draft(user)` to fail because drafts do not have a `get_draft` method.
> 
> you could solve this by making sure you don't have the draft above with `get_review_request()`

Could you provide more detail as to the change needed -- it's unclear after a quick look.
Comment on attachment 8801872 [details]
MozReview: Review Page Redesign: Update main content area contents (Bug 1309964).

https://reviewboard.mozilla.org/r/86476/#review91746

> Could you provide more detail as to the change needed -- it's unclear after a quick look.

Sure,

`self.review_request_details` is one of two things when this method is called, the `ReviewRequest` object, or if a draft exists and the user has access it's the `ReviewRequestDraft` object.

Many of our extra data helper methods, like `get_parent_rr` can take either a `ReviewRequest` or a `ReviewRequestDraft`. If you pass them a `ReviewRequestDraft` some will attempt to return the draft version of their return value. In the case of `get_parent_rr` it's a little weird, it looks like we always return the `ReviewRequest` not the `ReviewRequestDraft` *unless* what you're passing in is the parent itself. So if `self.review_request_details` *is* the parent, and is a `ReviewRequestDraft` you will just get the same thing back.

In that case the following call to `parent.get_draft(user)` will fail with an attribute error, since `ReviewRequestDraft` doesn't have a `get_draft()` method, only `ReviewRequest` does.
Comment on attachment 8808245 [details]
MozReview: Review Page Redesign: Add 'mozreview-review-content' template hook (Bug 1309964)

https://reviewboard.mozilla.org/r/91084/#review92952
Attachment #8808245 - Flags: review?(glob) → review+
Comment on attachment 8809522 [details]
MozReview: Review Page Redesign: Add reviewRequestEditorView_mozreview.js for future height calculation modification (Bug 1309964).

https://reviewboard.mozilla.org/r/92080/#review92954
Attachment #8809522 - Flags: review?(glob) → review+
Comment on attachment 8809523 [details]
MozReview: Review Page Redesign: Prevent reviewboard from calculating main content height (Bug 1309964).

https://reviewboard.mozilla.org/r/92082/#review92956
Attachment #8809523 - Flags: review?(glob) → review+
Comment on attachment 8801872 [details]
MozReview: Review Page Redesign: Update main content area contents (Bug 1309964).

https://reviewboard.mozilla.org/r/86476/#review93394

looking good; these are mostly nits.

::: pylib/mozreview/mozreview/extension.py:53
(Diff revision 10)
>      BaseCommitField,
>      CombinedReviewersField,
>      CommitAuthorField,
>      CommitsListField,
>      FileDiffReviewerField,
> -    ImportCommitField,
> +    CommitDetailField,

sort the import list please

::: pylib/mozreview/mozreview/fields.py:137
(Diff revision 10)
> -        super(ImportCommitField, self).__init__(review_request_details,
> +        """
> +        if not self.commit_id:
> +            logger.error('No commit_id for review request: %d' % (
> +                review_request.id))
> +        """

delete these lines instead of commenting out

::: pylib/mozreview/mozreview/fields.py:187
(Diff revision 10)
> +            'pull_text': pull_text,
> +            'import_text': import_text,

i like that the pull and import text is moved here :)
Attachment #8801872 - Flags: review?(glob) → review-
Comment on attachment 8804899 [details]
MozReview: Review Page Redesign: Enhance table toggling and add cookie table state (Bug 1309964).

https://reviewboard.mozilla.org/r/88728/#review93080

::: pylib/mozreview/mozreview/templates/mozreview/commits-requests.html:28
(Diff revision 9)
>      <td colspan="2">
>        {% if prev_child %}
>          <a href="{{prev_child.get_review_request.get_absolute_url}}" rel="previous">&lt;&lt; Previous Commit</a>
>        {% endif %}
>        Viewing commit {{current_child_num}} of {{num_children}}.
> -      <a href="javascript:;" id="mozreview-all-commits">View All Commits</a>
> +      <a href="javascript:;" data-one-text="View This Commit" data-all-text="View All Commits" id="mozreview-all-commits">{% if request.COOKIES.commits_table_show == 'true' %}View This Commit{% else %}View All Commits{% endif %}</a>

"View This Commit" and "View All Commits" could be clarified somewhat; i'm not sure the use of 'this' is clear enough.

how about

"Show Other Commits"
"Hide Other Commits"
Attachment #8804899 - Flags: review?(glob) → review-
Comment on attachment 8801872 [details]
MozReview: Review Page Redesign: Update main content area contents (Bug 1309964).

https://reviewboard.mozilla.org/r/86476/#review94452
Attachment #8801872 - Flags: review?(glob) → review+
Comment on attachment 8801339 [details]
MozReview: Review Page Redesign: Move commits table above the main content area (Bug 1309964).

https://reviewboard.mozilla.org/r/86118/#review94454
Attachment #8801339 - Flags: review?(glob) → review+
Blocks: 1234279
Comment on attachment 8801872 [details]
MozReview: Review Page Redesign: Update main content area contents (Bug 1309964).

https://reviewboard.mozilla.org/r/86476/#review94626

::: pylib/mozreview/mozreview/extension.py:286
(Diff revision 12)
> -            description_field.should_render = (lambda self, value:
> -                not is_parent(self.review_request_details))
> +            self.old_desc_should_render = description_field.should_render
> +            description_field.should_render = lambda value: False

Shouldn't this lambda take two arguments, the old one did... it surprises me this works?

::: pylib/mozreview/mozreview/fields.py:129
(Diff revision 12)
>                  'repository_path': review_request.repository.path
>              }))
>  
>  
> -class ImportCommitField(BaseReviewRequestField):
> -    """This field provides some information on how to pull down the commit"""
> +class CommitDetailField(BaseReviewRequestField):
> +    """This field provides the main content for reviews"""

s/reviews/review requests

::: pylib/mozreview/mozreview/fields.py:137
(Diff revision 12)
> +        if not self.commit_id:
> +            logger.error('No commit_id for review request: %d' % (
> +                review_request.id))

This will happen for *every* parent review request if I'm not mistaken, seems strange to log this? Do you even need to grab the commit id in `__init__`, since we have the commit data, fetching from it is a cheap operation.

::: pylib/mozreview/mozreview/fields.py:167
(Diff revision 12)
> -            commit_id = commit_data.extra_data.get(COMMIT_ID_KEY)
> +        import_text = pull_text = ""
> -
> -        review_request = self.review_request_details.get_review_request()
>          repo_path = review_request.repository.path
>  
> -        if not commit_id:
> +        import_text = ""

You've already set this to an empty string above.

::: pylib/mozreview/mozreview/fields.py:168
(Diff revision 12)
> -            logger.error('No commit_id for review request: %d' % (
> -                review_request.id))
> +        was_parent = is_parent(self.review_request_details,
> +                               self.commit_data)

please don't rely on the order of defaulted arguments, pass `self.commit_data` as `commit_data=self.commit_data`.

::: pylib/mozreview/mozreview/fields.py:168
(Diff revision 12)
> -
> -        review_request = self.review_request_details.get_review_request()
>          repo_path = review_request.repository.path
>  
> -        if not commit_id:
> -            logger.error('No commit_id for review request: %d' % (
> +        import_text = ""
> +        was_parent = is_parent(self.review_request_details,

`was_parent` is a really strange name to me, it makes me thinking you're changing whether it is the parent in the future. You don't actually need the check for `not was_parent` when doing the import text, the `commit_id` is enough, so why don't you scrap this variable and make the `is_parent()` call part of the pull conditional.

::: pylib/mozreview/mozreview/fields.py:175
(Diff revision 12)
> -            return ''
> +        if self.commit_id and not was_parent:
> +            import_text = "hg import %s/rev/%s" % (repo_path, self.commit_id)
>  
> -        return get_template('mozreview/hg-pull.html').render(Context({
> -                'commit_id': commit_id,
> -                'repo_path': repo_path,
> +        pull_commit_id = self.commit_id
> +        if was_parent:
> +            pull_commit_data = fetch_commit_data(children_details[-1])

I think something like `last_child_commit_data` is more informative.
Attachment #8801872 - Flags: review?(smacleod) → review+
Comment on attachment 8804899 [details]
MozReview: Review Page Redesign: Enhance table toggling and add cookie table state (Bug 1309964).

https://reviewboard.mozilla.org/r/88728/#review94634

Glob's handling the review for this.
Attachment #8804899 - Flags: review?(smacleod)
Comment on attachment 8812816 [details]
MozReview: Review Page Redesign: Update main content area to show full commit message (Bug 1309964).

https://reviewboard.mozilla.org/r/94408/#review94636

r+ if pyflakes errors are fixed.

::: pylib/mozreview/mozreview/fields.py:181
(Diff revision 1)
>              pull_commit_id = pull_commit_data.extra_data.get(COMMIT_ID_KEY)
>  
>          pull_text = "hg pull -r %s %s" % (pull_commit_id, repo_path)
>  
> +        # Get just the extended commit message details for display
> +        commit_message_detail = "\n".join(self.review_request_details.description.split("\n")[1:]).strip()

is this not > 80 characters? please run pyflakes and ensure you're not introducing errors (goes for the earlier commits too)

::: pylib/mozreview/mozreview/fields.py:181
(Diff revision 1)
>              pull_commit_id = pull_commit_data.extra_data.get(COMMIT_ID_KEY)
>  
>          pull_text = "hg pull -r %s %s" % (pull_commit_id, repo_path)
>  
> +        # Get just the extended commit message details for display
> +        commit_message_detail = "\n".join(self.review_request_details.description.split("\n")[1:]).strip()

`splitlines()` instead of `split("\n")`
Attachment #8812816 - Flags: review?(smacleod) → review+
Comment on attachment 8801872 [details]
MozReview: Review Page Redesign: Update main content area contents (Bug 1309964).

https://reviewboard.mozilla.org/r/86476/#review94676

::: pylib/mozreview/mozreview/fields.py:140
(Diff revision 13)
> -    # RB validation requires this to be unique, so we fake a field id
> -    field_id = "p2rb.PullCommitField"
> -    label = _("Pull")
>  
> -    def __init__(self, review_request_details, *args, **kwargs):
> -        self.commit_data = fetch_commit_data(review_request_details)
> +        commit_data = fetch_commit_data(self.review_request_details)
> +        commit_id = commit_data.extra_data.get(COMMIT_ID_KEY)

Bah, totally forgot to say, this will get the wrong commit_id if we're dealing with a draft (the submitter is viewing the page before publishing). You want `commit_data.get_for(self.review_request_details, COMMIT_ID_KEY)`
Comment on attachment 8812816 [details]
MozReview: Review Page Redesign: Update main content area to show full commit message (Bug 1309964).

https://reviewboard.mozilla.org/r/94408/#review94678

::: pylib/mozreview/mozreview/fields.py:172
(Diff revision 2)
> +        commit_message_detail = ("\n".join(self.review_request_details
> +                                    .description.splitlines()[1:]).strip())

Generally in Python you want to go with something like:
```
        commit_message_detail = "\n".join(
            self.review_request_details.description.splitlines()[1:]).strip()
```

Instead of the javascript style.
Comment on attachment 8801872 [details]
MozReview: Review Page Redesign: Update main content area contents (Bug 1309964).

https://reviewboard.mozilla.org/r/86476/#review94682

::: pylib/mozreview/mozreview/fields.py:166
(Diff revision 13)
> +            last_child_commit_id = (last_child_commit_data.extra_data
> +                                        .get(COMMIT_ID_KEY))

```
            last_child_commit_id = (
                last_child_commit_data.extra_data.get(COMMIT_ID_KEY))
```
Comment on attachment 8804899 [details]
MozReview: Review Page Redesign: Enhance table toggling and add cookie table state (Bug 1309964).

https://reviewboard.mozilla.org/r/88728/#review94902

Please remove my name from the commit message of this commit so it stops reflagging me.
Attachment #8804899 - Flags: review?(smacleod)
Comment on attachment 8812816 [details]
MozReview: Review Page Redesign: Update main content area to show full commit message (Bug 1309964).

https://reviewboard.mozilla.org/r/94408/#review94944

All issues have been fixed here but still seeing a "!1", so no idea what's up with that.
Comment on attachment 8804899 [details]
MozReview: Review Page Redesign: Enhance table toggling and add cookie table state (Bug 1309964).

https://reviewboard.mozilla.org/r/88728/#review95258

This is ready for a look, glob.
Comment on attachment 8812816 [details]
MozReview: Review Page Redesign: Update main content area to show full commit message (Bug 1309964).

https://reviewboard.mozilla.org/r/94408/#review95534

::: pylib/mozreview/mozreview/templates/mozreview/commit-main.html:50
(Diff revision 4)
> +{% if commit_message_detail %}
> +  <pre class="review-request-details">{{ commit_message_detail }}</pre>
> +{% endif %}

this ends up with text-align:center - it should be left-aligned.
Attachment #8812816 - Flags: review?(glob) → review-
Comment on attachment 8804899 [details]
MozReview: Review Page Redesign: Enhance table toggling and add cookie table state (Bug 1309964).

https://reviewboard.mozilla.org/r/88728/#review95536

i'm seeing issues with the 'always show all commits' cookie...

steps:
1. delete the commits_table_show cookie
2. refresh the page
3. check the 'always show all commits' checkbox
4. refresh the page

the checkbox is unchecked after the page is loaded. it appears the cookie isn't created when the checkbox is clicked.  if i uncheck the checkbox the cookie is created, and things work correctly from there on.
Attachment #8804899 - Flags: review?(glob) → review-
Comment on attachment 8804899 [details]
MozReview: Review Page Redesign: Enhance table toggling and add cookie table state (Bug 1309964).

https://reviewboard.mozilla.org/r/88728/#review95536

Fixed!
Comment on attachment 8812816 [details]
MozReview: Review Page Redesign: Update main content area to show full commit message (Bug 1309964).

https://reviewboard.mozilla.org/r/94408/#review95948

Updated to left-align the the extended info.  Will beautify more in the future.
Comment on attachment 8801339 [details]
MozReview: Review Page Redesign: Move commits table above the main content area (Bug 1309964).

https://reviewboard.mozilla.org/r/86118/#review95950

glob or smacleod:  Can one of you santiy check something for me?  Whenever I r+ one commit in the table locally, (then of course the review dropdown dialog closes), the "r+/r?/!1" issue badge doesn't switch to "r+", despite using the same code as the pre-redesign (`{% elif child_details.get_review_request.approved %}`).  Have I done something wrong or ... ?
Comment on attachment 8801339 [details]
MozReview: Review Page Redesign: Move commits table above the main content area (Bug 1309964).

https://reviewboard.mozilla.org/r/86118/#review96374

::: pylib/mozreview/mozreview/templates/mozreview/commits-requests.html:50
(Diff revision 18)
> +    {% endif %}>
>  
>    <td class="commits">
>      <div class="truncate_text">
> +      {% if child_details.get_review_request.issue_open_count > 0 %}
> +        <div class="status approval-issues" title="{{child_details.get_review_request.issue_open_count}} open issues">

i'm sorry; i missed this in my previous reviews: this says "1 open issues" instead of "1 open issue" when there is a single issue.
Attachment #8801339 - Flags: review+ → review-
Comment on attachment 8804899 [details]
MozReview: Review Page Redesign: Enhance table toggling and add cookie table state (Bug 1309964).

https://reviewboard.mozilla.org/r/88728/#review96378

i still see the same problem when the cookie isn't initially set.
Attachment #8804899 - Flags: review?(glob) → review-
Comment on attachment 8812816 [details]
MozReview: Review Page Redesign: Update main content area to show full commit message (Bug 1309964).

https://reviewboard.mozilla.org/r/94408/#review96482
Attachment #8812816 - Flags: review?(glob) → review+
Comment on attachment 8804899 [details]
MozReview: Review Page Redesign: Enhance table toggling and add cookie table state (Bug 1309964).

https://reviewboard.mozilla.org/r/88728/#review96492

Cookie initalization fixed by passing a string instead of a boolean, despite the fact that you set the initial values with a boolean.  FFS.
Comment on attachment 8801339 [details]
MozReview: Review Page Redesign: Move commits table above the main content area (Bug 1309964).

https://reviewboard.mozilla.org/r/86118/#review97014
Attachment #8801339 - Flags: review?(glob) → review+
Comment on attachment 8804899 [details]
MozReview: Review Page Redesign: Enhance table toggling and add cookie table state (Bug 1309964).

https://reviewboard.mozilla.org/r/88728/#review97016
Attachment #8804899 - Flags: review?(glob) → review+
i haven't autolanded this because i don't think that's wise to do just before the all-hands when our availability to address issues from this major change is limited (i'm not expecting issue, but..).

this should be autolanded and pushed as soon as possible after the all-hands.
Pushed by bjones@mozilla.com:
https://hg.mozilla.org/webtools/reviewboard/rev/e8ee0bfac509
MozReview: Review Page Redesign: Add user session cookie for commits table persistence . r=glob
https://hg.mozilla.org/webtools/reviewboard/rev/05b49b352b25
MozReview: Review Page Redesign: Add review templates for future 'mozreview-review-content' modification . r=glob
https://hg.mozilla.org/webtools/reviewboard/rev/2291f7702824
MozReview: Review Page Redesign: Add 'mozreview-review-content' template hook r=glob,smacleod
https://hg.mozilla.org/webtools/reviewboard/rev/0095c5a6ad8c
MozReview: Review Page Redesign: Add reviewRequestEditorView_mozreview.js for future height calculation modification . r=glob
https://hg.mozilla.org/webtools/reviewboard/rev/ce5e6e0bbed3
MozReview: Review Page Redesign: Prevent reviewboard from calculating main content height . r=glob
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Pushed by bjones@mozilla.com:
https://hg.mozilla.org/hgcustom/version-control-tools/rev/2ba95814a149
MozReview: Review Page Redesign: Move commits table above the main content area . r=glob,smacleod
https://hg.mozilla.org/hgcustom/version-control-tools/rev/5c0c6b21ddf5
MozReview: Review Page Redesign: Update main content area contents . r=glob,smacleod
https://hg.mozilla.org/hgcustom/version-control-tools/rev/0249f68ba9cf
MozReview: Review Page Redesign: Enhance table toggling and add cookie table state . r=glob
https://hg.mozilla.org/hgcustom/version-control-tools/rev/4e0742f5454f
MozReview: Review Page Redesign: Update main content area to show full commit message . r=glob,smacleod
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: