-
Notifications
You must be signed in to change notification settings - Fork 102
Mobile view for the Interop Dashboard #4458
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.
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
base: main
Are you sure you want to change the base?
Conversation
2cf3e08
to
2fb2fe4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with some nits to consider.
calculateColor(score) { | ||
const gradient = [ | ||
// Red. | ||
{ scale: 0, color: [250, 0, 0] }, | ||
// Orange. | ||
{ scale: 33.33, color: [250, 125, 0] }, | ||
// Yellow. | ||
{ scale: 66.67, color: [220, 220, 0] }, | ||
// Green. | ||
{ scale: 100, color: [0, 160, 0] }, | ||
]; | ||
|
||
let color1, color2; | ||
for (let i = 1; i < gradient.length; i++) { | ||
if (score <= gradient[i].scale) { | ||
color1 = gradient[i - 1]; | ||
color2 = gradient[i]; | ||
break; | ||
} | ||
} | ||
const colorWeight = ((score - color1.scale) / (color2.scale - color1.scale)); | ||
const color = [ | ||
Math.round(color1.color[0] * (1 - colorWeight) + color2.color[0] * colorWeight), | ||
Math.round(color1.color[1] * (1 - colorWeight) + color2.color[1] * colorWeight), | ||
Math.round(color1.color[2] * (1 - colorWeight) + color2.color[2] * colorWeight), | ||
]; | ||
|
||
return [ | ||
`rgb(${color[0]}, ${color[1]}, ${color[2]})`, | ||
`rgba(${color[0]}, ${color[1]}, ${color[2]}, 0.15)`, | ||
]; | ||
} | ||
|
||
getSubtotalScoreStyle(section, stable) { | ||
const interopIndex = this.dataManager.getYearProp('numBrowsers'); | ||
const score = this.getNumericalSubtotalScore(interopIndex, section, stable); | ||
const colors = this.calculateColor(score); | ||
return `color: color-mix(in lch, ${colors[0]} 70%, black); background-color: ${colors[1]}`; | ||
} | ||
|
||
getScoreStyle(feature, stable) { | ||
const interopIndex = this.dataManager.getYearProp('numBrowsers'); | ||
const score = this.getNumericalBrowserScoreByFeature(interopIndex, feature, stable); | ||
const colors = this.calculateColor(score); | ||
return `color: color-mix(in lch, ${colors[0]} 70%, black); background-color: ${colors[1]}`; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think some tests for these lines would be helpful (or at least calculateColor). There are a lot of possibilities going on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely worth having some tests here. Additionally, there's a similar test in interop-summary.js
. I've moved this function to the utils file and added tests for it. 🙂
webapp/components/interop-summary.js
Outdated
@@ -209,7 +234,9 @@ class InteropSummary extends PolymerElement { | |||
} | |||
|
|||
const summaryDiv = this.shadowRoot.querySelector('.summary-container'); | |||
summaryDiv.style.minHeight = SUMMARY_CONTAINER_MIN_HEIGHTS[this.year] || '470px'; | |||
if (window.innerWidth > 800) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason to not move this all to CSS rules now?
Something like:
.summary-container {
min-height: 470px;
}
@media (min-width: 801px) {
.summary-container {
min-height: 470px;
}
.summary-container[data-year="2023"] {
min-height: 500px; /* Example height for year 2023 */
}
.summary-container[data-year="2024"] {
min-height: 520px; /* Example height for year 2024 */
}
/* ... and so on for other years */
}
I know there are other places where we do CSS styling in JavaScript but instead of adding another layer with the conditional to keep track of here, we could move it to CSS styles.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this is a great way to handle this - I've updated my implementation with this approach. 🙂
f89a875
to
ba63939
Compare
@foolip I'm guessing this is a change we would want to run by the interop team before landing to make sure there are no objections. Perhaps you would also like to take a look? |
Staging link
This change adds a mobile-friendly view to the Interop dashboard. wpt.fyi has had essentially no mobile-friendly functionality, and the mobile view is simply a shrunken view of the desktop version of the site.
This change should have little to no effect on the desktop view of the site (with the exception of some small adjustments to the logo and title placement). Additionally, no other mobile view of the site except the Interop dashboard should be changed in any way.
Interop dashboard changes:
Before
Screen.recording.2025-06-23.4.54.25.PM.webm
After
Screen.recording.2025-06-23.4.51.59.PM.webm