Closed
Bug 1419075
Opened 7 years ago
Closed 7 years ago
Create a button that open a sidebar in the console
Categories
(DevTools :: Console, enhancement, P2)
DevTools
Console
Tracking
(firefox59 fixed)
RESOLVED
FIXED
Firefox 59
Tracking | Status | |
---|---|---|
firefox59 | --- | fixed |
People
(Reporter: nchevobbe, Assigned: mpark)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
The button can go next to the "filter" one to start with, and will be available only when building from source.
What should be done :
1. Create a "sidebarToggle" action like https://searchfox.org/mozilla-central/rev/c633ffa4c4611f202ca11270dcddb7b29edddff8/devtools/client/webconsole/new-console-output/actions/ui.js#21-29 . We don't have to deal with the preference bit for now.
2. Add a `sideBarVisible` property in the state https://searchfox.org/mozilla-central/rev/c633ffa4c4611f202ca11270dcddb7b29edddff8/devtools/client/webconsole/new-console-output/reducers/ui.js#22
3. Add a reducer handler for the action in ui.js, similar to the one for filterBarTogg;e https://searchfox.org/mozilla-central/rev/c633ffa4c4611f202ca11270dcddb7b29edddff8/devtools/client/webconsole/new-console-output/reducers/ui.js#31-32 , setting the `sideBarVisible` property
4. In the ConsoleOutput mapStateToProps (https://searchfox.org/mozilla-central/source/devtools/client/webconsole/new-console-output/components/ConsoleOutput.js#179-191), get the value of the `sideBarVisible` state value (state.ui.sideBarVisible)
5. In the ConsoleOutput component we render all the messages: https://searchfox.org/mozilla-central/source/devtools/client/webconsole/new-console-output/components/ConsoleOutput.js#141-165 . Here we should check if the sideBarVisible props is truthy, and if so , happen a div with the word "SideBar WIP" inside for example.
This should change the dom hierarchy from :
div.webconsole-output
.message
.message
to
div.webconsole-output
div.messages-list
.message
.message
aside.sidebar
And change the CSS in webconsole.css, on the part that is dedicated to the new console https://searchfox.org/mozilla-central/rev/c633ffa4c4611f202ca11270dcddb7b29edddff8/devtools/client/themes/webconsole.css#753
When everything is done, it would be nice to test that the reducer react like it should to the sidebarToggle action like we do for other reducer (see https://searchfox.org/mozilla-central/rev/c633ffa4c4611f202ca11270dcddb7b29edddff8/devtools/client/webconsole/new-console-output/test/store/search.test.js#8-25), creating a new ui.test.js test file.
Reporter | ||
Updated•7 years ago
|
Assignee: nobody → mpark
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Reporter | ||
Updated•7 years ago
|
Severity: normal → enhancement
Priority: -- → P2
Reporter | ||
Comment 2•7 years ago
|
||
mozreview-review |
Comment on attachment 8930216 [details]
Bug 1419075 - Create a button that open a sidebar in the console.
https://reviewboard.mozilla.org/r/201368/#review206770
This is a really good start :)
There is some failure when we run `cd devtools/client/webconsole && npm test`, related to the filter bar (which makes sense, since we modify it here).
We miss 2 things here, one from you, one from me :
1. We do not want to show the button to the user yet. So maybe we can have a pref in order to display it.
2. I was wrong telling you to put the sidebar in the ConsoleOutput, it should be upper. A good fit would be https://searchfox.org/mozilla-central/rev/919dce54f43356c22d6ff6b81c07ef412b1bf933/devtools/client/webconsole/new-console-output/new-console-output-wrapper.js#198, after `childComponent` (which can be renamed to consoleOutput).
This means that we need to create a new component, in a new file, and connect it to the store so we can get the state.ui.sideBarVisible value. You can copy ConsoleOutput as an example.
Now, if we do that, we have 3 elements as siblings:
FilterBar
ConsoleOutput
SideBar
It could be nice to display webconsole-output-wrapper as a grid with 2 rows and 2 columns :
+-------------------------+------------+
| FILTER BAR | |
+-------------------------+ |
| | |
| | |
| | SIDEBAR |
| | |
| CONSOLEOUTPUT | |
| | |
| | |
| | |
| | |
+-------------------------+------------+
The Sidebar would then span across rows (grid-row: 1 / -1).
You can have a look at https://codepen.io/nchevobbe/pen/dZmbqV , clicking on the document toggle the visibility of the sidebar, and you can see that the filter and messages elements take the whole space when the sidebar is hidden.
::: devtools/client/themes/webconsole.css:1188
(Diff revision 1)
> +
> +.messages-list {
> + float: left;
> +}
> +
> +.sidebar {
> + float: right;
> + position: sticky;
> + top: 0;
> +}
We could have used flexbox here. But since we are going to place the sidebar upper in the dom, we won't need this.
Also, we should look at other sidebars in the devtools and see if they have a special CSS class to style them (so we have a border, and maybe a different background color)
::: devtools/client/webconsole/new-console-output/actions/ui.js:63
(Diff revision 1)
> + return (dispatch, getState) => {
> + dispatch({
> + type: SIDEBAR_TOGGLE,
> + });
> + };
for now, we could simply return the object :
```
return {
type: SIDEBAR_TOGGLE,
};
```
We use a callback (in redux world, a Thunk action) when we need to have access to the state, or when we have multiple async function to call.
::: devtools/client/webconsole/new-console-output/components/FilterBar.js:249
(Diff revision 1)
> + dom.button({
> + className: "devtools-button devtools-filter-icon",
> + title: l10n.getStr("webconsole.toggleFilterButton.tooltip"),
> + onClick: this.onClickSidebarToggle
> + }),
could you move it at the very end of the bar, not use the devtools-filter-icon class, and display a "Toggle SideBar" text ?
Attachment #8930216 -
Flags: review?(nchevobbe) → review-
Comment 3•7 years ago
|
||
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #2)
> 1. We do not want to show the button to the user yet. So maybe we can have a
> pref in order to display it.
We could also put `display: none` on it in webconsole.css and then override that in launchpad so that it's visible only in development
Comment hidden (mozreview-request) |
Reporter | ||
Comment 5•7 years ago
|
||
mozreview-review |
Comment on attachment 8930216 [details]
Bug 1419075 - Create a button that open a sidebar in the console.
https://reviewboard.mozilla.org/r/201368/#review207060
This is getting close, it probably be good to land in the next round :)
::: devtools/client/themes/webconsole.css:1200
(Diff revision 2)
> +.hidden {
> + display: none;
> +}
I think this can be removed
::: devtools/client/webconsole/new-console-output/components/FilterBar.js:264
(Diff revision 2)
> + dom.button({
> + className: "devtools-button" + (
> + Services.prefs.getBoolPref(PREFS.UI.SIDEBAR_TOGGLE) ? "" : " hidden"),
> + title: l10n.getStr("webconsole.toggleFilterButton.tooltip"),
> + onClick: this.onClickSidebarToggle
> + }, "Toggle Sidebar"),
we have a prefs stata where this would better fit.
See how we already do this for the logLimit (https://searchfox.org/mozilla-central/rev/797c93d81fe446f78babf20894f0729f15f71ee6/devtools/client/webconsole/new-console-output/store.js#42-43,46 & https://searchfox.org/mozilla-central/rev/797c93d81fe446f78babf20894f0729f15f71ee6/devtools/client/webconsole/new-console-output/reducers/prefs.js#9-11)
This is set only at startup, which is fine.
::: devtools/client/webconsole/new-console-output/components/SideBar.js:23
(Diff revision 2)
> + return (
> + dom.aside({
> + className: "sidebar" + (sidebarVisible ? "" : " hidden"),
> + }, "Sidebar WIP")
> + );
usually we prefer not to render something instead of adding a className. e.g.
```
return (
sidebarVisible
? dom.aside({
className: "sidebar",
}, "Sidebar WIP")
: null
);
```
::: devtools/client/webconsole/package.json:21
(Diff revision 2)
> + "devtools-source-editor": "=0.0.3",
> "enzyme": "^2.4.1",
> "expect": "^1.16.0",
> "file-loader": "^0.10.1",
> "immutable": "^3.8.1",
> "jsdom": "^9.4.1",
> "jsdom-global": "^2.0.0",
> "json-loader": "^0.5.4",
> "mocha": "^2.5.3",
> "raw-loader": "^0.5.1",
> "react": "=15.3.2",
> "react-addons-perf": "=15.3.2",
> "react-dom": "=15.3.2",
> "react-redux": "=5.0.3",
> "redux": "^3.6.0",
> "require-hacker": "^2.1.4",
> + "reselect": "^3.0.1",
you should be able to rebase on top of mozilla-central to get rid of this
Attachment #8930216 -
Flags: review?(nchevobbe) → review-
Comment hidden (mozreview-request) |
Reporter | ||
Comment 7•7 years ago
|
||
mozreview-review |
Comment on attachment 8930216 [details]
Bug 1419075 - Create a button that open a sidebar in the console.
https://reviewboard.mozilla.org/r/201368/#review207308
I tested it in the launchpad as well as in the toolbox with and without the pref, everything works well.
The mocha tests are all green, so I think this is ready to land.
::: devtools/client/webconsole/new-console-output/components/FilterBar.js:301
(Diff revision 3)
> return {
> filter: getAllFilters(state),
> filterBarVisible: uiState.filterBarVisible,
> persistLogs: uiState.persistLogs,
> filteredMessagesCount: getFilteredMessagesCount(state),
> + sidebarToggle: state.prefs.sidebarToggle
nit: add a trailing comma so next diffs does not pick this line
Attachment #8930216 -
Flags: review?(nchevobbe) → review+
Comment hidden (mozreview-request) |
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fc4036a8b9e8
Create a button that open a sidebar in the console. r=nchevobbe
Comment 10•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•