Bug 1602431 Comment 14 Edit History

Note: The actual edited comment in the bug view page will always show the original commenter’s name and original timestamp.

Review of attachment 9121409 [details] [diff] [review]:
-----------------------------------------------------------------

From a UI point of view this works as expected, thanks for taking care of this.
I left a little comment for a portion of code, but of course let's wait for Magnus' review.

Also, and I don't know if it's a local issue, I can't load your patch due to the special character in the changeset header
`# User Thomas Düllmann <bugzilla2007@duellmann24.net>`

::: mail/base/content/mailWidgets.js
@@ +2057,5 @@
> +          }
> +        }
> +      }
> +      return null;
> +    }

This is a bit verbose and can be simplified by removing the `else` statements.
```
if (siblingsType == "next") {
  // Check for next siblings.
  let element = this.nextElementSibling;
  while (element) {
    if (!element.hasAttribute("selected")) {
      return element
    }
    element = element.nextElementSibling;
  }
}
	
// Check for previous siblings.
let element = this.previousElementSibling;
while (element) {
  if (!element.hasAttribute("selected")) {
    return element
  }
  element = element.previousElementSibling;
}
	
return null;
```
Review of attachment 9121409 [details] [diff] [review]:
-----------------------------------------------------------------

From a UI point of view this works as expected, thanks for taking care of this.
I left a little comment for a portion of code, but of course let's wait for Magnus' review.

Also, and I don't know if it's a local issue, I can't load your patch due to the special character in the changeset header
`# User Thomas Düllmann <bugzilla2007[spam]>`

::: mail/base/content/mailWidgets.js
@@ +2057,5 @@
> +          }
> +        }
> +      }
> +      return null;
> +    }

This is a bit verbose and can be simplified by removing the `else` statements.
```
if (siblingsType == "next") {
  // Check for next siblings.
  let element = this.nextElementSibling;
  while (element) {
    if (!element.hasAttribute("selected")) {
      return element
    }
    element = element.nextElementSibling;
  }
}
	
// Check for previous siblings.
let element = this.previousElementSibling;
while (element) {
  if (!element.hasAttribute("selected")) {
    return element
  }
  element = element.previousElementSibling;
}
	
return null;
```
Review of attachment 9121409 [details] [diff] [review]:
-----------------------------------------------------------------

From a UI point of view this works as expected, thanks for taking care of this.
I left a little comment for a portion of code, but of course let's wait for Magnus' review.

Also, and I don't know if it's a local issue, I can't load your patch due to the special character in the changeset header
`# User Thomas Düllmann <bugzilla2007**[snip]**>`

::: mail/base/content/mailWidgets.js
@@ +2057,5 @@
> +          }
> +        }
> +      }
> +      return null;
> +    }

This is a bit verbose and can be simplified by removing the `else` statements.
```
if (siblingsType == "next") {
  // Check for next siblings.
  let element = this.nextElementSibling;
  while (element) {
    if (!element.hasAttribute("selected")) {
      return element
    }
    element = element.nextElementSibling;
  }
}
	
// Check for previous siblings.
let element = this.previousElementSibling;
while (element) {
  if (!element.hasAttribute("selected")) {
    return element
  }
  element = element.previousElementSibling;
}
	
return null;
```

Back to Bug 1602431 Comment 14