Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2770743004:
DevTools: improve $0 selected element hint in elements (Closed)
|
Created:
3 years, 9 months ago by luoe Modified:
3 years, 8 months ago CC:
chromium-reviews, caseq+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, blink-reviews, kozyatinskiy+blink_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionDevTools: improve $0 selected element hint in elements
This adds a tooltip on the '== $0' hint explaining what it is for.
BUG=703002
Review-Url: https://codereview.chromium.org/2770743004
Cr-Commit-Position: refs/heads/master@{#461860}
Committed: https://chromium.googlesource.com/chromium/src/+/ebbfebff49abb37df9789e93cb1feb29dd399941
Patch Set 1 #Patch Set 2 : ac #
Total comments: 4
Patch Set 3 : ac #
Total comments: 3
Patch Set 4 : ac #
Messages
Total messages: 26 (12 generated)
Description was changed from ========== DevTools: improve $0 selected element hint in elements BUG=703002 ========== to ========== DevTools: improve $0 selected element hint in elements Following this CL: - Tooltip on the hint explaining what it is - Hint text is now '== $0 in console' - Clicking on the hint evaluates '$0' in the console BUG=703002 ==========
luoe@chromium.org changed reviewers: + lushnikov@chromium.org, pfeldman@chromium.org
pfeldman, lushnikov, wdyt? Is adding the words 'in console' fine? Please take a look.
Is there a screenshot?
There is now! I added screenshots in the crbug http://crbug.com/703002
Description was changed from ========== DevTools: improve $0 selected element hint in elements Following this CL: - Tooltip on the hint explaining what it is - Hint text is now '== $0 in console' - Clicking on the hint evaluates '$0' in the console BUG=703002 ========== to ========== DevTools: improve $0 selected element hint in elements This adds a tooltip on the '== $0' hint explaining what it is for. BUG=703002 ==========
Removed extra stuff, now it's just adding tooltip text.
https://codereview.chromium.org/2770743004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/elements/ElementsTreeElement.js (right): https://codereview.chromium.org/2770743004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/ElementsTreeElement.js:240: var hintElement = listItemElement.createChild('div', 'selected-hint'); you gonna re-create this multiple times on .updateTitle() https://codereview.chromium.org/2770743004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/elements/elementsTreeOutline.css (right): https://codereview.chromium.org/2770743004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/elementsTreeOutline.css:41: .elements-disclosure li:not(.selected) .selected-hint:before { you don't need half of these styles; let's make a less invasive solution
ptal https://codereview.chromium.org/2770743004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/elements/ElementsTreeElement.js (right): https://codereview.chromium.org/2770743004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/ElementsTreeElement.js:240: var hintElement = listItemElement.createChild('div', 'selected-hint'); On 2017/03/24 22:05:11, lushnikov wrote: > you gonna re-create this multiple times on .updateTitle() I've added a check to make sure a hintElement doesn't already exist first. I've also pulled out _createHint() to avoid unnecessary hint making on hover. createSelection() in the hovered setter, which we don't want to trigger building a hint element. https://codereview.chromium.org/2770743004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/elements/elementsTreeOutline.css (right): https://codereview.chromium.org/2770743004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/elementsTreeOutline.css:41: .elements-disclosure li:not(.selected) .selected-hint:before { Extra styles removed. https://codereview.chromium.org/2770743004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/elements/ElementsTreeElement.js (right): https://codereview.chromium.org/2770743004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/ElementsTreeElement.js:1064: } Drive by: It turns out that making tons of search queries creates tons of unnecessary selectionElements in the dom! Since searching doesn't affect selection, we should be doing delete/create only inside this branch.
https://codereview.chromium.org/2770743004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/elements/elementsTreeOutline.css (left): https://codereview.chromium.org/2770743004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/elementsTreeOutline.css:36: .elements-disclosure li.selected.editing-as-html:after { these classes served an important role of making '== $0 ' white when selected. How do you solve this now?
Please take another look https://codereview.chromium.org/2770743004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/elements/elementsTreeOutline.css (left): https://codereview.chromium.org/2770743004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/elements/elementsTreeOutline.css:36: .elements-disclosure li.selected.editing-as-html:after { After removing the 'color: black' style, the selected-hint text color inherits from the default platform color, which is a less intense black. Doing so changes the current color slightly, but also lets the 'color: white' style on Line 123 apply here. I think we can simplify and remove this one-off style, even if the hint text is no longer black. As for the display: none, it's now being hidden without the need for this CSS rule. ElementsTreeElement.js does this to its children: while (child) { child.style.display = 'none'; child = child.nextSibling; } That said, the 'editing-as-html' class is no longer used so I'll remove that!
Gentle ping
lgtm
The CQ bit was checked by luoe@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by luoe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by luoe@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1491342725506150,
"parent_rev": "29aa3bc7a1fb3b50e9610e35be6eb052f60e4696", "commit_rev":
"ebbfebff49abb37df9789e93cb1feb29dd399941"}
Message was sent while issue was closed.
Description was changed from ========== DevTools: improve $0 selected element hint in elements This adds a tooltip on the '== $0' hint explaining what it is for. BUG=703002 ========== to ========== DevTools: improve $0 selected element hint in elements This adds a tooltip on the '== $0' hint explaining what it is for. BUG=703002 Review-Url: https://codereview.chromium.org/2770743004 Cr-Commit-Position: refs/heads/master@{#461860} Committed: https://chromium.googlesource.com/chromium/src/+/ebbfebff49abb37df9789e93cb1f... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/ebbfebff49abb37df9789e93cb1f... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
