Commit c29911fd authored by Greg Tatum's avatar Greg Tatum
Browse files

Bug 1597376 - Make the summary dropdowns conditional in the profiler settings; r=julienw

This is probably the biggest change to the existing components, as it makes the
summary dropdowns conditional based on the page context. This keeps the old
workflow working, but allows for the new about:profiling page's design. Most
of the diff here is creating the new _renderSection method which consolidates
this logic, and also handles the summary div structure.

Differential Revision: https://phabricator.services.mozilla.com/D55009

--HG--
extra : moz-landing-system : lando
parent 6cba996a
...@@ -12,6 +12,7 @@ ...@@ -12,6 +12,7 @@
* @property {string} threadsString * @property {string} threadsString
* @property {string[]} objdirs * @property {string[]} objdirs
* @property {string[] | null} supportedFeatures * @property {string[] | null} supportedFeatures
* @property {PageContext} pageContext
*/ */
/** /**
...@@ -36,6 +37,7 @@ ...@@ -36,6 +37,7 @@
* @typedef {import("../@types/perf").PopupWindow} PopupWindow * @typedef {import("../@types/perf").PopupWindow} PopupWindow
* @typedef {import("../@types/perf").State} StoreState * @typedef {import("../@types/perf").State} StoreState
* @typedef {StateProps & DispatchProps} Props * @typedef {StateProps & DispatchProps} Props
* @typedef {import("../@types/perf").PageContext} PageContext
*/ */
/** /**
...@@ -63,6 +65,7 @@ const { ...@@ -63,6 +65,7 @@ const {
label, label,
input, input,
span, span,
h1,
h2, h2,
h3, h3,
section, section,
...@@ -78,6 +81,7 @@ const { ...@@ -78,6 +81,7 @@ const {
makeExponentialScale, makeExponentialScale,
formatFileSize, formatFileSize,
calculateOverhead, calculateOverhead,
UnhandledCaseError,
} = require("devtools/client/performance-new/utils"); } = require("devtools/client/performance-new/utils");
const { connect } = require("devtools/client/shared/vendor/react-redux"); const { connect } = require("devtools/client/shared/vendor/react-redux");
const actions = require("devtools/client/performance-new/store/actions"); const actions = require("devtools/client/performance-new/store/actions");
...@@ -403,6 +407,59 @@ class Settings extends PureComponent { ...@@ -403,6 +407,59 @@ class Settings extends PureComponent {
this.setState({ temporaryThreadText: null }); this.setState({ temporaryThreadText: null });
this.props.changeThreads(_threadTextToList(event.target.value)); this.props.changeThreads(_threadTextToList(event.target.value));
} }
/**
* about:profiling doesn't need to collapse the children into details/summary,
* but the popup and devtools do (for now).
*
* @param {string} id
* @param {React.ReactNode} title
* @param {React.ReactNode} children
* @returns React.ReactNode
*/
_renderSection(id, title, children) {
const { pageContext } = this.props;
switch (pageContext) {
case "popup":
case "devtools":
// Render the section with a dropdown summary.
return details(
{
className: "perf-settings-details",
// @ts-ignore - The React type definitions don't know about onToggle.
onToggle: _handleToggle,
},
summary(
{
className: "perf-settings-summary",
id,
},
// Concatenating strings like this isn't very localizable, but it should go
// away by the time we localize these components.
title + ":"
),
// Contain the overflow of the slide down animation with the first div.
div(
{ className: "perf-settings-details-contents" },
// Provide a second <div> element for the contents of the slide down
// animation.
div(
{ className: "perf-settings-details-contents-slider" },
children
)
)
);
case "aboutprofiling":
// Render the section without a dropdown summary.
return div(
{ className: "perf-settings-sections" },
div(null, h2(null, title), children)
);
default:
throw new UnhandledCaseError(pageContext, "PageContext");
}
}
/** /**
* @param {ThreadColumn[]} threadDisplay * @param {ThreadColumn[]} threadDisplay
* @param {number} index * @param {number} index
...@@ -435,27 +492,11 @@ class Settings extends PureComponent { ...@@ -435,27 +492,11 @@ class Settings extends PureComponent {
_renderThreads() { _renderThreads() {
const { temporaryThreadText } = this.state; const { temporaryThreadText } = this.state;
return this._renderSection(
return details( "perf-settings-threads-summary",
{ "Threads",
className: "perf-settings-details",
// @ts-ignore - The React type definitions don't know about onToggle.
onToggle: _handleToggle,
},
summary(
{
className: "perf-settings-summary",
id: "perf-settings-threads-summary",
},
"Threads:"
),
// Contain the overflow of the slide down animation with the first div.
div(
{ className: "perf-settings-details-contents" },
// Provide a second <div> element for the contents of the slide down
// animation.
div( div(
{ className: "perf-settings-details-contents-slider" }, null,
div( div(
{ className: "perf-settings-thread-columns" }, { className: "perf-settings-thread-columns" },
threadColumns.map(this._renderThreadsColumns) threadColumns.map(this._renderThreadsColumns)
...@@ -471,7 +512,7 @@ class Settings extends PureComponent { ...@@ -471,7 +512,7 @@ class Settings extends PureComponent {
"be only a partial match of the thread name to be included. It " + "be only a partial match of the thread name to be included. It " +
"is whitespace sensitive.", "is whitespace sensitive.",
}, },
div({}, "Add custom threads by name:"), div(null, "Add custom threads by name:"),
input({ input({
className: "perf-settings-text-input", className: "perf-settings-text-input",
id: "perf-settings-thread-text", id: "perf-settings-thread-text",
...@@ -487,7 +528,6 @@ class Settings extends PureComponent { ...@@ -487,7 +528,6 @@ class Settings extends PureComponent {
) )
) )
) )
)
); );
} }
...@@ -517,6 +557,8 @@ class Settings extends PureComponent { ...@@ -517,6 +557,8 @@ class Settings extends PureComponent {
className: `perf-settings-checkbox-label perf-settings-feature-label ${extraClassName}`, className: `perf-settings-checkbox-label perf-settings-feature-label ${extraClassName}`,
key: value, key: value,
}, },
div(
{ className: "perf-settings-checkbox-and-name" },
input({ input({
className: "perf-settings-checkbox", className: "perf-settings-checkbox",
id: `perf-settings-feature-checkbox-${value}`, id: `perf-settings-feature-checkbox-${value}`,
...@@ -526,7 +568,8 @@ class Settings extends PureComponent { ...@@ -526,7 +568,8 @@ class Settings extends PureComponent {
onChange: this._handleFeaturesCheckboxChange, onChange: this._handleFeaturesCheckboxChange,
disabled: !isSupported, disabled: !isSupported,
}), }),
div({ className: "perf-settings-feature-name" }, name), div({ className: "perf-settings-feature-name" }, name)
),
div( div(
{ className: "perf-settings-feature-title" }, { className: "perf-settings-feature-title" },
title, title,
...@@ -547,23 +590,11 @@ class Settings extends PureComponent { ...@@ -547,23 +590,11 @@ class Settings extends PureComponent {
} }
_renderFeatures() { _renderFeatures() {
return details( return this._renderSection(
{ "perf-settings-features-summary",
className: "perf-settings-details", "Features",
// @ts-ignore - The React type definitions don't know about onToggle.
onToggle: _handleToggle,
},
summary(
{
className: "perf-settings-summary",
id: "perf-settings-features-summary",
},
"Features:"
),
div(
{ className: "perf-settings-details-contents" },
div( div(
{ className: "perf-settings-details-contents-slider" }, null,
// Render the supported features first. // Render the supported features first.
featureCheckboxes.map(featureCheckbox => featureCheckboxes.map(featureCheckbox =>
this._renderFeatureCheckbox(featureCheckbox, false) this._renderFeatureCheckbox(featureCheckbox, false)
...@@ -577,29 +608,16 @@ class Settings extends PureComponent { ...@@ -577,29 +608,16 @@ class Settings extends PureComponent {
this._renderFeatureCheckbox(featureCheckbox, true) this._renderFeatureCheckbox(featureCheckbox, true)
) )
) )
)
); );
} }
_renderLocalBuildSection() { _renderLocalBuildSection() {
const { objdirs } = this.props; const { objdirs } = this.props;
return details( return this._renderSection(
{ "perf-settings-local-build-summary",
className: "perf-settings-details", "Local build",
// @ts-ignore - The React type definitions don't know about onToggle.
onToggle: _handleToggle,
},
summary(
{
className: "perf-settings-summary",
id: "perf-settings-local-build-summary",
},
"Local build:"
),
div(
{ className: "perf-settings-details-contents" },
div( div(
{ className: "perf-settings-details-contents-slider" }, null,
p( p(
null, null,
`If you're profiling a build that you have compiled yourself, on this `If you're profiling a build that you have compiled yourself, on this
...@@ -612,15 +630,37 @@ class Settings extends PureComponent { ...@@ -612,15 +630,37 @@ class Settings extends PureComponent {
onRemove: this._handleRemoveObjdir, onRemove: this._handleRemoveObjdir,
}) })
) )
)
); );
} }
/**
* For now, render different titles depending on the context.
* @return {string}
*/
_renderTitle() {
const { pageContext } = this.props;
switch (pageContext) {
case "aboutprofiling":
return "Buffer Settings";
case "popup":
case "devtools":
return "Recording Settings";
default:
throw new UnhandledCaseError(pageContext, "PageContext");
}
}
render() { render() {
return section( return section(
{ className: "perf-settings" }, { className: "perf-settings" },
h2({ className: "perf-settings-title" }, "Recording Settings"), this.props.pageContext === "aboutprofiling"
div( ? h1(null, "Full Settings")
: null,
h2({ className: "perf-settings-title" }, this._renderTitle()),
// The new about:profiling will implement a different overhead mechanism.
this.props.pageContext === "aboutprofiling"
? null
: div(
{ className: "perf-settings-row" }, { className: "perf-settings-row" },
label({ className: "perf-settings-label" }, "Overhead:"), label({ className: "perf-settings-label" }, "Overhead:"),
div( div(
...@@ -710,6 +750,7 @@ function mapStateToProps(state) { ...@@ -710,6 +750,7 @@ function mapStateToProps(state) {
threadsString: selectors.getThreadsString(state), threadsString: selectors.getThreadsString(state),
objdirs: selectors.getObjdirs(state), objdirs: selectors.getObjdirs(state),
supportedFeatures: selectors.getSupportedFeatures(state), supportedFeatures: selectors.getSupportedFeatures(state),
pageContext: selectors.getPageContext(state),
}; };
} }
......
...@@ -254,6 +254,10 @@ ...@@ -254,6 +254,10 @@
display: flex; display: flex;
} }
.perf-settings-checkbox-and-name {
display: flex;
}
.perf-settings-checkbox { .perf-settings-checkbox {
align-self: flex-start; align-self: flex-start;
} }
......
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment