Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add warmup and step delay to developer UI #323

Merged
merged 12 commits into from
Feb 29, 2024
Merged

Add warmup and step delay to developer UI #323

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
82c05c4
adding warmup and step delay to deverloper UI
camillobruni Oct 4, 2023
951a4e1
Merge branch 'main' into 2023-10-04_delay_params
camillobruni Nov 7, 2023
64e1e23
undo moving
camillobruni Nov 7, 2023
23687f5
Merge branch 'main' into 2023-10-04_delay_params
camillobruni Nov 24, 2023
780f91b
fix default
camillobruni Nov 24, 2023
9817ff2
adding max iterations
camillobruni Nov 30, 2023
b64dc03
updating metric
camillobruni Nov 30, 2023
7ff287a
Merge branch 'main' into 2023-10-04_delay_params
camillobruni Jan 16, 2024
195c57e
address comment
camillobruni Jan 16, 2024
dc13bec
fixes
camillobruni Jan 16, 2024
30977c9
updating developer mode params and settings code.
camillobruni Jan 16, 2024
aa38532
Merge branch 'main' into 2023-10-04_delay_params
camillobruni Feb 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 58 additions & 20 deletions resources/developer-mode.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,16 @@ export function createDeveloperModeContainer() {
content.append(createUIForSuites());

content.append(document.createElement("hr"));
content.append(createUIForIterationCount());
content.append(document.createElement("br"));
camillobruni marked this conversation as resolved.
Show resolved Hide resolved
content.append(createUIForMeasurementMethod());
content.append(document.createElement("br"));
content.append(createUIForWarmupSuite());
content.append(document.createElement("br"));
content.append(createUIForIterationCount());
content.append(createUIForWarmupBeforeSync())
content.append(document.createElement("br"));
content.append(createUIForSyncStepDelay());
details.append(content);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

details.append(content) is already done below, so probably this line should be removed


content.append(document.createElement("hr"));
content.append(createUIForRun());
Expand All @@ -29,7 +34,7 @@ export function createDeveloperModeContainer() {
return container;
}

export function createUIForMeasurementMethod() {
function createUIForMeasurementMethod() {
let check = document.createElement("input");
check.type = "checkbox";
check.id = "measurement-method";
Expand All @@ -46,7 +51,7 @@ export function createUIForMeasurementMethod() {
return label;
}

export function createUIForWarmupSuite() {
function createUIForWarmupSuite() {
let check = document.createElement("input");
check.type = "checkbox";
check.id = "warmup-suite";
Expand All @@ -63,33 +68,56 @@ export function createUIForWarmupSuite() {
return label;
}

export function createUIForIterationCount() {
let range = document.createElement("input");
range.type = "range";
range.id = "iteration-count";
range.min = 1;
range.max = 20;
range.value = params.iterationCount;

let label = document.createElement("label");
let countLabel = document.createElement("span");
countLabel.textContent = params.iterationCount;
label.append("iterations: ", countLabel, document.createElement("br"), range);

range.oninput = () => {
countLabel.textContent = range.value;
function createUIForIterationCount() {
const { range, label } = createTimeRangeUI("Iterations: ", params.iterationCount, "#", 1, 100);
camillobruni marked this conversation as resolved.
Show resolved Hide resolved
range.onchange = () => {
params.iterationCount = parseInt(range.value);
updateURL();
};
return label;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like we could factorize a bit more these UI creations, but I'm not sure this would actually be easier to read. So I'm fine with keeping them this way.


function createUIForWarmupBeforeSync() {
const { range, label } = createTimeRangeUI("Warmup time: ", params.warmupBeforeSync);
range.onchange = () => {
params.iterationCount = parseInt(range.value);
params.warmupBeforeSync = parseInt(range.value);
updateURL();
};
return label;
}

function createUIForSyncStepDelay() {
const { range, label } = createTimeRangeUI("Sync step delay: ", params.waitBeforeSync);
range.onchange = () => {
params.waitBeforeSync = parseInt(range.value);
updateURL();
};
return label;
}

function createTimeRangeUI(labelText, initialValue, unit = "ms", min = 0, max = 1000) {
const range = document.createElement("input");
range.type = "range";
range.min = min;
range.max = max;
range.value = initialValue;

const label = document.createElement("label");
const rangeLabel = document.createElement("span");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: the name "label" can be confusing in this context, I believe it should be renamed, for example to rangeValueText?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if rangeLabel should be outside of the label element (it's not labeling the input), but this would need more CSS changes that we're willing to do here, I believe.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed the variable to rangeValueAndUnit, let's maybe move the rest for later.

rangeLabel.className = "range-label-data";
const rangeValue = document.createElement("span");
rangeLabel.append(rangeValue, " ", unit);
rangeValue.textContent = initialValue;
label.append(labelText, range, rangeLabel);

export function createUIForSuites() {
range.oninput = () => {
rangeValue.textContent = range.value;
};

return { range, label };
}

function createUIForSuites() {
const control = document.createElement("nav");
const ol = document.createElement("ol");
const checkboxes = [];
Expand Down Expand Up @@ -246,6 +274,16 @@ function updateURL() {
else
url.searchParams.delete("useWarmupSuite");

if (params.warmupBeforeSync !== defaultParams.warmupBeforeSync)
url.searchParams.set("warmupBeforeSync", params.warmupBeforeSync);
else
url.searchParams.delete("warmupBeforeSync");

if (params.waitBeforeSync !== defaultParams.waitBeforeSync)
url.searchParams.set("waitBeforeSync", params.waitBeforeSync);
else
url.searchParams.delete("waitBeforeSync");

camillobruni marked this conversation as resolved.
Show resolved Hide resolved
// Only push state if changed
url.search = decodeURIComponent(url.search);
if (url.href !== window.location.href)
Expand Down
8 changes: 8 additions & 0 deletions resources/main.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -245,6 +245,14 @@ button,
gap: 3px;
}

.developer-mode-content input[type="range"],
.developer-mode-content .range-label-data {
float: right;
}
.developer-mode-content .range-label-data {
padding-right: 0.5em;
}

.developer-mode-content li + li {
margin-top: 0px;
}
Expand Down