Closed Bug 1359759 Opened 8 years ago Closed 8 years ago

The grid outline triggers a lot of React warnings and needs some polishing

Categories

(DevTools :: Inspector, defect, P3)

defect

Tracking

(firefox55 fixed)

RESOLVED FIXED
Firefox 55
Tracking Status
firefox55 --- fixed

People

(Reporter: pbro, Assigned: pbro)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

In bug 1345300 comment 6, Jen suggested that the grid outline should be at the bottom of the grid panel. I agree with that. After having used the tool for some time, it's driving me mad that the checkboxes jump down when the outline appears. Also, when the outline appears, React warnings are logged. When you mouseover the outline, more warnings are logged. Finally, when you mouseover cells in the outline, it kind of feels really slow.
I have a bunch of commits locally that address these issues, making the outline seem generally much faster than it is today, and fixing a bunch of re-renders that don't need to happen.
Assignee: nobody → pbrosset
Blocks: 1347964
Status: NEW → ASSIGNED
Blocks: dt-grid
> After having used the tool for some time, it's driving me mad that the checkboxes jump down when the outline appears. That's been my experience, too. The more I use the tool, the more frustrating the jumping around becomes. (It's a perfect example of why designs are not done after they've been drawn in a static-drawing tool like Photoshop or Sketch. We needed to see how the interaction worked with real content to see this problem.) Let's do move the grid outline diagram to the bottom. Also — anytime I move my mouse around, I seem to rollover this diagram, which triggers highlighting I didn't intend. And lot of times the highlighting stays on. Somehow the cell boxes don't loose state on roll-off, and then multiple cells are highlighted. I end up with overlay boxes all over my web page. And I have to spend time going around turning off the overlays, but usually that triggers more unwanted overlays... when I didn't intend any of it, I was just moving my mouse across the screen. I'm hoping this is all simply because it's early days. Perhaps these are the same bugs you are describing above, Patrick. Hopefully the rollover will wait several milliseconds to trigger, so that it only activates when desired.
Priority: -- → P3
(In reply to Jen Simmons [:jensimmons] from comment #2) > Also — anytime I move my mouse around, I seem to rollover this diagram, > which triggers highlighting I didn't intend. And lot of times the > highlighting stays on. Somehow the cell boxes don't loose state on roll-off, > and then multiple cells are highlighted. I end up with overlay boxes all > over my web page. And I have to spend time going around turning off the > overlays, but usually that triggers more unwanted overlays... when I didn't > intend any of it, I was just moving my mouse across the screen. Yes, I noticed that as well and I have a patch that addresses this issue which will be part of this bug too.
(In reply to Jen Simmons [:jensimmons] from comment #2) > That's been my experience, too. The more I use the tool, the more > frustrating the jumping around becomes. (It's a perfect example of why > designs are not done after they've been drawn in a static-drawing tool like > Photoshop or Sketch. We needed to see how the interaction worked with real > content to see this problem.) > > Let's do move the grid outline diagram to the bottom. Gabriel: I know we've discussed about this yesterday and you said you were concerned about one use case: what if there are many grids in the page and the outline is hidden below the fold, never to be found by users? I think that based on Jen's feedback above, we should still move it down, and consider (for now) the case where there are too many grids as an edge case. I think moving the outline down is an improvement in the majority of cases. If it turns out it's frustrating to users, we can iterate and think of a better way: - either have a max-height and an overflow:scroll on the list of grids, - or think of a better layout like 2 columns side by side, 1 for the list of grids, one for the outline. And then we could move the 2 settings checkboxes to a cog icon drop-down menu somewhere, like our other settings. In the meantime, I have uploaded a patch for moving it down.
Flags: needinfo?(gl)
Comment on attachment 8864448 [details] Bug 1359759 - 1 - Move grid outline below grid details to avoid UI jumps; https://reviewboard.mozilla.org/r/136136/#review140250
Attachment #8864448 - Flags: review?(gl) → review+
Comment on attachment 8864449 [details] Bug 1359759 - 2 - Clear React warnings in GridOutline and prevent re-renders; https://reviewboard.mozilla.org/r/136138/#review140902 ::: devtools/client/inspector/grids/components/GridOutline.js:57 (Diff revision 1) > // lagging the grid cell highlighting if a lot of grid cells are mouseover in a > // quick succession. > this.highlightCell = throttle(this.highlightCell, GRID_CELL_MOUSEOVER_TIMEOUT); > }, > > componentWillReceiveProps({ grids }) { It seems like we can change up the code in this function just so we can set the state once. ::: devtools/client/inspector/grids/components/GridOutline.js:78 (Diff revision 1) > + width: totalWidth, > - }); > + }); > + } > + }, > + > + getTotalWidthAndHeight(grid) { Add a JSDoc describing what this function is doing ::: devtools/client/inspector/grids/grid-inspector.js:307 (Diff revision 1) > + this.lastHighlighterColor = null; > + this.lastHighlighterNode = null; > + this.lastHighlighterState = null; > + }, > + > + showGridHighlighter(node, settings) { Move showGridHighlighter and toggleGridHilighter functions below loadHighlighterSettings. We try to keep functions alphabetically ordered.
Attachment #8864449 - Flags: review?(gl) → review+
Comment on attachment 8864450 [details] Bug 1359759 - 3 - Faster debounce for grid highlighting; https://reviewboard.mozilla.org/r/136140/#review140914 ::: devtools/client/inspector/grids/components/GridOutline.js:136 (Diff revision 1) > - highlightCell({ target }) { > + highlightCell(e) { > + // Debounce the highlighting of cells. > + // This way we don't end up sending many requests to the server for highlighting when > + // cells get hovered in a rapid succession We only send a request if the user settles > + // on a cell for some time. > + if (this.windowResizeTimer) { windowResizeTimer seems like the wrong name. Perhaps highlightTimeout?
Attachment #8864450 - Flags: review?(gl) → review+
Blocks: 1348471
Flags: needinfo?(gl)
Pushed by pbrosset@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/719e52d35af4 1 - Move grid outline below grid details to avoid UI jumps; r=gl https://hg.mozilla.org/integration/autoland/rev/7c4b3082b544 2 - Clear React warnings in GridOutline and prevent re-renders; r=gl https://hg.mozilla.org/integration/autoland/rev/bdc6784cdfd4 3 - Faster debounce for grid highlighting; r=gl
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: