Bugfixes and minor improvements #858
Conversation
src/js/techreport/timeseries.js
Outdated
| label.innerHTML = latest.technology; | ||
| if(latestValue) { | ||
| value.innerHTML = `${latestValue}${config.series.suffix || ''}`; | ||
| if(summary) { | ||
| value.innerHTML = `${summaryValue}`; | ||
| secondaryValue.innerHTML = `${latestValue}${config.series.suffix || ''}`; | ||
| } else { | ||
| value.innerHTML = `${latestValue}${config.series.suffix || ''}`; | ||
| } | ||
| } else { | ||
| value.classList.add('undefined'); | ||
| value.innerHTML = 'No data'; |
There was a problem hiding this comment.
Nit but we've recently discovered innerHTML (and innerText) is actually a bit slower than textContent.
It if really is just text (and I'm not sure if it always is), could we use textContent?
There was a problem hiding this comment.
So far it's only text, can update to textContent
src/js/techreport/utils/data.js
Outdated
| client: 'desktop', | ||
| date: date, | ||
| }, | ||
| mobile: { | ||
| ...submetric.mobile, | ||
| median_bytes_formatted: submetric.mobile.median_bytes > 1024 ? `${Math.round(submetric.mobile.median_bytes / 1024)} KB` : submetric.mobile.median_bytes > 1048576 ? `${Math.round(submetric.mobile.median_bytes / 1048576)} MB` : `${submetric.mobile.median_bytes} bytes`, |
There was a problem hiding this comment.
Same comment. (should this conversion be moved to a reusable function?)
There was a problem hiding this comment.
Good point, will move to a function in the utils 👍🏻
| @@ -39,13 +39,15 @@ <h4> | |||
| <div class="breakdown-item"> | |||
| <p class="breakdown-label">{{ breakdown.name }}</p> | |||
| <p class="breakdown-value">00{% if breakdown.suffix == "%" %}.00{% endif %}{{ breakdown.suffix }}</p> | |||
| <p class="breakdown-value-secondary"></p> | |||
There was a problem hiding this comment.
Is this only needed because the chart is always in bytes? TBH I don't love the redundancy and I think we can get away with only showing the formatted value. Alternatively, WDYT about stuffing the # of bytes into the title attribute of the formatted value?
There was a problem hiding this comment.
I removed it for now since the title comes with accessibility challenges and is easily overlooked anyway. If people need to hover to see the value they can just as well hover over the graph (which works with keyboard etc) 😄
If we get feedback that it's confusing that one is in MB/KB and the other one in bytes, then maybe we can add a custom (accessible) tooltip on the formatted values.
Co-authored-by: Rick Viscomi <[email protected]>
…nto cwvtech-bugfixes
|
@rviscomi updated :) |
Summary of the changes
Changed from reported bugfixes:
Solves issue: Beta Report - Confusion about where the data is #851
Solves issue: Improve tech report page weight formatting #853
Solives issue: Improve tech report page weight formatting #853
Solves issue: Tech report data tooltips show datetime in local timezone #849
Solves issue: Update beta header #854
Solves issue: Beta Report - Blank Dropdown List Items in Dark Theme #850
Solves issue: Upgrade to production tech report API #864
Additional changes:
Screenshots
Timezone San Francisco - main branch
Timezone San Francisco - changes
