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)

enhancement

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.
Blocks: 1419076
Blocks: 1419078
Assignee: nobody → mpark
Status: NEW → ASSIGNED
Blocks: 1419081
Blocks: 1419087
Blocks: 1419088
Severity: normal → enhancement
Priority: -- → P2
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-
(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 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 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+
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
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: