JSHint
-
reviewboard/static/rb/js/views/reviewRequestEditorView.es6.js (Diff revision 1) Show all issues
Review Request #12734 — Created Nov. 22, 2022 and submitted
Information | |
---|---|
david | |
Review Board | |
release-6.x | |
Reviewers | |
reviewboard | |
This change adds the unified banner, a major new UI feature in Review
Board 6.This new banner replaces several existing banners:
The goal here is twofold. First, we want to make it very clear whenever
there's any unpublished work (often review reply drafts can get lost in
long pages). Second, we want to enable people to publish everything all
in one go (the typical use case being publishing a bunch of review
replies and a review request update all together).
This also gives us room for future work:
This is mostly complete. The current things left on my to-do list are:
Summary | |
---|---|
Description | From | Last Updated |
---|---|---|
'unifiedBanner' is defined but never used. Column: 15 Error code: W098 |
![]() |
|
Duplicate key 'click #action-add-general-comment'. Column: 44 Error code: W075 |
![]() |
|
This is probably true of other previously-reviewed versions as well, but looking more closely at it, this text is really … |
|
|
Missing period. |
|
|
Can we keep this in alphabetical order? |
|
|
Thinking it might be worth fleshing this out a bit. There's a lot going on in this banner, so just … |
|
|
Tiny nit: Can we have a blank line between multi-line list items? Helps keep it all from visually blending together … |
|
|
This line is too long. |
|
|
This is missing a doc comment. |
|
|
Ordering is off. Any ID-based selectors should go after the &__ parts. |
|
|
&__mode should have a doc comment somewhere, even if the ruleset is empty. This will be important for the doc … |
|
|
Missing a doc comment. |
|
|
Modifier rules should go before any child rules, up near the top, since they apply to the main component. |
|
|
This could go in &__menu (though you would need to replace the &__mode with the full name). Might be a … |
|
|
Let's standardize on a blank line after this comment, so there's always room/incentive for per-variable comments. In this case, though, … |
|
|
Can we say how this will update it? |
|
|
These are missing docs. |
|
|
Can we use numDrafts or draftCount instead? The n prefix has always felt too much like shorthand to me. |
|
|
Same comment about the n prefix. |
|
|
Can we have a comment describing this particular condition? There's a lot to keep track of in these. Might be … |
|
|
Can we add a file-level doc comment? |
|
|
Wonder if we should treat other bundles as a separate import group from stuff in the current bundle. Imports in … |
|
|
Missing a Version Added. Here and below. |
|
|
Let's standardize on a consistent blank line after the "Instance variables" block. I also realized, from working with the AST … |
|
|
These should be in alphabetical order. |
|
|
Completely optional (mostly using this comment to point it out the capability), but we do have this now: this.#menuView.renderInto(this.$el); Instead … |
|
|
This event handler isn't returning anything. |
|
|
Let's stop defaults/propagation here and below before we do anything else, so errors don't cause events to bubble. |
|
|
Maybe worth adding a clearItems() method to MenuView? I feel like this shouldn't be reaching into internals, or we're asking … |
|
|
We can probably skip this check, since if it's 0, the for loop is a no-op. |
|
|
Rather than querying this every time we update, let's pull this out during render and just access it via a … |
|
|
We fetch draftModes[i].text in both conditions, so we could just pull it into a variable once. That also simplifies the … |
|
|
Let's pull this out as well. |
|
|
There's a space between change: and draftModes here. Might need more testing on that. |
|
|
Blank line here. |
|
|
Missing a doc comment. |
|
|
The summary must be one line. |
|
|
This can use :js:attr |
|
|
We should use the specific element for the banner. Is that just a div? |
|
|
Missing space between > and {. |
|
|
Can we type this? |
|
|
There will be things to figure out with some of the references cross-bundle, but I think you want: :js:class:`RB.FloatingBannerView` |
|
|
No return value needed here. |
|
|
No need to call the parent. That's explicitly empty. |
|
|
If any of these are direct children, let's use this.$el.children('...') to avoid the deep lookup. Same elsewhere. |
|
|
We access this.model a bunch. Let's pull it out into a local variable. |
|
|
This can be: this.#modeMenu.renderInto(this.#modeSelector); |
|
|
Can we separate these? Keep the publish button stuff together? |
|
|
No return value needed here. |
|
|
If we're going to log, let's be more explicit about what wasn't rendered, since the log output won't have context. … |
|
|
Instead of repeated attribute lookups, let's pull out this.model into a local variable. |
|
|
Can we use numDrafts or draftCount instead of the n prefix? |
|
|
Since we're already using setVisible, this could be: this.#discardButton.setVisible( draftModes.length > 0 && !draftModes[selectedDraftMode].multiple); |
|
|
We should add an interface for this. |
|
|
Let's pull out this.model into a local variable. |
|
|
Can we type these? |
|
|
For this call, let's have two groups of keys (separated by a blank line): Options/data for the API call Callbacks … |
|
|
We access this.model a lot in this function, so let's pull it out. |
|
Summary: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Testing Done: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Commits: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 2 (+3592 -234) |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Added Files: |
reviewboard/static/rb/js/pages/views/reviewablePageView.es6.js (Diff revision 2) |
---|
Duplicate key 'click #action-add-general-comment'. Column: 44 Error code: W075
Commits: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 3 (+3602 -234) |
Description: |
|
---|
This is awesome, David. I'm so happy to see this becoming reality.
There's a lot of comments here, but most are in the categories of:
- Doc improvements/missing docs
- Couple repeated naming suggestions
- Reducing unnecessary attribute accesses
- Missing types
- Just small typos and such
The core of it looks really good. Nothing major anywhere, just polish. I can't wait to use this :)
reviewboard/reviews/actions.py (Diff revision 3) |
---|
This is probably true of other previously-reviewed versions as well, but looking more closely at it, this text is really more about the base class's version and less about this specific implementation. Can you update it to describe how this class determines if the menu should be rendered?
reviewboard/static/rb/css/pages/review-request.less (Diff revision 3) |
---|
Can we keep this in alphabetical order?
reviewboard/static/rb/css/ui/banners.less (Diff revision 3) |
---|
Thinking it might be worth fleshing this out a bit. There's a lot going on in this banner, so just having an overview of how it all fits together will help keep this maintainable.
reviewboard/static/rb/css/ui/banners.less (Diff revision 3) |
---|
Ordering is off. Any ID-based selectors should go after the
&__
parts.
reviewboard/static/rb/css/ui/banners.less (Diff revision 3) |
---|
&__mode
should have a doc comment somewhere, even if the ruleset is empty.This will be important for the doc generation stuff I'm presently toying with.
reviewboard/static/rb/css/ui/banners.less (Diff revision 3) |
---|
Modifier rules should go before any child rules, up near the top, since they apply to the main component.
Let's standardize on a blank line after this comment, so there's always room/incentive for per-variable comments.
In this case, though, I don't know that
template
is strictly an instance variable. I mean, it is, but in much the same way thatclassName
is. I think this section is best left for things that are set/mutated during the lifetime of an instance, not what are effectively constants.
reviewboard/static/rb/js/reviews/views/reviewRequestActions.ts (Diff revision 3) |
---|
Can we say how this will update it?
reviewboard/static/rb/js/reviews/models/unifiedBanner.ts (Diff revision 3) |
---|
Can we use
numDrafts
ordraftCount
instead? Then
prefix has always felt too much like shorthand to me.
reviewboard/static/rb/js/reviews/models/unifiedBanner.ts (Diff revision 3) |
---|
Same comment about the
n
prefix.
reviewboard/static/rb/js/reviews/models/unifiedBanner.ts (Diff revision 3) |
---|
Can we have a comment describing this particular condition? There's a lot to keep track of in these.
Might be nice to actually have one per-condition.
reviewboard/static/rb/js/reviews/views/unifiedBannerView.ts (Diff revision 3) |
---|
Can we add a file-level doc comment?
reviewboard/static/rb/js/reviews/views/unifiedBannerView.ts (Diff revision 3) |
---|
Wonder if we should treat other bundles as a separate import group from stuff in the current bundle.
Imports in JavaScript feel so much more difficult to clearly organize. I haven't really found a really nice pattern I like, except that I've been loosely doing:
- Platform (for Node)
- Third-party (node_modules)
- Current bundle
And I could see a 2.5 in there that's "other bundles in same project." Or maybe that just goes into third-party. I don't know. Thoughts?
reviewboard/static/rb/js/reviews/views/unifiedBannerView.ts (Diff revision 3) |
---|
Missing a
Version Added
.Here and below.
reviewboard/static/rb/js/reviews/views/unifiedBannerView.ts (Diff revision 3) |
---|
Let's standardize on a consistent blank line after the "Instance variables" block.
I also realized, from working with the AST for TypeScript this past week, that this will prevent
#menuView
from being assigned that comment block as its leading comment.
reviewboard/static/rb/js/reviews/views/unifiedBannerView.ts (Diff revision 3) |
---|
Completely optional (mostly using this comment to point it out the capability), but we do have this now:
this.#menuView.renderInto(this.$el);
Instead of the
.append()
and.render()
.
reviewboard/static/rb/js/reviews/views/unifiedBannerView.ts (Diff revision 3) |
---|
This event handler isn't returning anything.
reviewboard/static/rb/js/reviews/views/unifiedBannerView.ts (Diff revision 3) |
---|
Let's stop defaults/propagation here and below before we do anything else, so errors don't cause events to bubble.
reviewboard/static/rb/js/reviews/views/unifiedBannerView.ts (Diff revision 3) |
---|
Maybe worth adding a
clearItems()
method toMenuView
? I feel like this shouldn't be reaching into internals, or we're asking for trouble down the road.
reviewboard/static/rb/js/reviews/views/unifiedBannerView.ts (Diff revision 3) |
---|
We can probably skip this check, since if it's 0, the
for
loop is a no-op.
reviewboard/static/rb/js/reviews/views/unifiedBannerView.ts (Diff revision 3) |
---|
Rather than querying this every time we update, let's pull this out during render and just access it via a variable. We update more than we render, especially since this is in a loop.
reviewboard/static/rb/js/reviews/views/unifiedBannerView.ts (Diff revision 3) |
---|
We fetch
draftModes[i].text
in both conditions, so we could just pull it into a variable once. That also simplifies the template literal.
reviewboard/static/rb/js/reviews/views/unifiedBannerView.ts (Diff revision 3) |
---|
Let's pull this out as well.
reviewboard/static/rb/js/reviews/views/unifiedBannerView.ts (Diff revision 3) |
---|
There's a space between
change:
anddraftModes
here. Might need more testing on that.
reviewboard/static/rb/js/reviews/views/unifiedBannerView.ts (Diff revision 3) |
---|
Missing a doc comment.
reviewboard/static/rb/js/reviews/views/unifiedBannerView.ts (Diff revision 3) |
---|
The summary must be one line.
reviewboard/static/rb/js/reviews/views/unifiedBannerView.ts (Diff revision 3) |
---|
We should use the specific element for the banner. Is that just a div?
reviewboard/static/rb/js/reviews/views/unifiedBannerView.ts (Diff revision 3) |
---|
Missing space between
>
and{
.
reviewboard/static/rb/js/reviews/views/unifiedBannerView.ts (Diff revision 3) |
---|
There will be things to figure out with some of the references cross-bundle, but I think you want:
:js:class:`RB.FloatingBannerView`
reviewboard/static/rb/js/reviews/views/unifiedBannerView.ts (Diff revision 3) |
---|
No return value needed here.
reviewboard/static/rb/js/reviews/views/unifiedBannerView.ts (Diff revision 3) |
---|
No need to call the parent. That's explicitly empty.
reviewboard/static/rb/js/reviews/views/unifiedBannerView.ts (Diff revision 3) |
---|
If any of these are direct children, let's use
this.$el.children('...')
to avoid the deep lookup.Same elsewhere.
reviewboard/static/rb/js/reviews/views/unifiedBannerView.ts (Diff revision 3) |
---|
We access
this.model
a bunch. Let's pull it out into a local variable.
reviewboard/static/rb/js/reviews/views/unifiedBannerView.ts (Diff revision 3) |
---|
This can be:
this.#modeMenu.renderInto(this.#modeSelector);
reviewboard/static/rb/js/reviews/views/unifiedBannerView.ts (Diff revision 3) |
---|
Can we separate these? Keep the publish button stuff together?
reviewboard/static/rb/js/reviews/views/unifiedBannerView.ts (Diff revision 3) |
---|
No return value needed here.
reviewboard/static/rb/js/reviews/views/unifiedBannerView.ts (Diff revision 3) |
---|
If we're going to log, let's be more explicit about what wasn't rendered, since the log output won't have context.
Or should this be an assert at this point?
reviewboard/static/rb/js/reviews/views/unifiedBannerView.ts (Diff revision 3) |
---|
Instead of repeated attribute lookups, let's pull out
this.model
into a local variable.
reviewboard/static/rb/js/reviews/views/unifiedBannerView.ts (Diff revision 3) |
---|
Can we use
numDrafts
ordraftCount
instead of then
prefix?
reviewboard/static/rb/js/reviews/views/unifiedBannerView.ts (Diff revision 3) |
---|
Since we're already using
setVisible
, this could be:this.#discardButton.setVisible( draftModes.length > 0 && !draftModes[selectedDraftMode].multiple);
reviewboard/static/rb/js/reviews/views/unifiedBannerView.ts (Diff revision 3) |
---|
We should add an interface for this.
reviewboard/static/rb/js/reviews/views/unifiedBannerView.ts (Diff revision 3) |
---|
Let's pull out
this.model
into a local variable.
reviewboard/static/rb/js/reviews/views/unifiedBannerView.ts (Diff revision 3) |
---|
For this call, let's have two groups of keys (separated by a blank line):
- Options/data for the API call
- Callbacks
That'll help keep this a lot more readable while also keeping ESLint happy.
reviewboard/static/rb/js/reviews/views/unifiedBannerView.ts (Diff revision 3) |
---|
We access
this.model
a lot in this function, so let's pull it out.
Commits: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 4 (+3710 -234) |
Commits: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 5 (+3734 -234) |
reviewboard/static/rb/css/ui/banners.less (Diff revisions 3 - 5) |
---|
Tiny nit: Can we have a blank line between multi-line list items? Helps keep it all from visually blending together as much.
reviewboard/static/rb/css/ui/banners.less (Diff revisions 3 - 5) |
---|
This could go in
&__menu
(though you would need to replace the&__mode
with the full name). Might be a bit better just for organization.
reviewboard/static/rb/js/reviews/views/unifiedBannerView.ts (Diff revisions 3 - 5) |
---|
These should be in alphabetical order.
onInitialRender
not calling the superclass (FloatingBannerView
) implementation.Commits: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 6 (+3750 -240) |