-
Notifications
You must be signed in to change notification settings - Fork 31
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
feat: Monaco theming #1560
feat: Monaco theming #1560
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1560 +/- ##
==========================================
+ Coverage 46.34% 46.48% +0.13%
==========================================
Files 569 571 +2
Lines 35916 36011 +95
Branches 8997 9012 +15
==========================================
+ Hits 16647 16738 +91
- Misses 19217 19221 +4
Partials 52 52
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
@bmingles I should add some more context to this quote on the CONTRIBUTING.md:
At the time I didn't think lerna was supporting this correctly, but upon re-reading this comment about the issue, we just need to set the changelog-preset: https://github.com/lerna/lerna/tree/main/libs/commands/version#--changelog-preset |
@bmingles scratch that, I tried making a PR to use the correct preset: #1565 |
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.
Minor cleanup suggestion.
// Remove the temporary css variables | ||
tmpPropEl.remove(); | ||
|
||
log.debug('Resolved css variables', performance.now() - perfStart, 'ms'); |
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.
Side note - perhaps we should add a .time
method to our Logger, that wraps console.time
: https://developer.mozilla.org/en-US/docs/Web/API/console/time
I would put that at a finer grain of detail than debug2
even. Just noting it.
return { | ||
r, | ||
g, | ||
b, | ||
a, | ||
}; |
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'd much prefer without the custom line breaks here
return { | |
r, | |
g, | |
b, | |
a, | |
}; | |
return { r, g, b, a }; |
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.
ah yeah. I'll fix on next PR
--dh-color-xxx-hue
variable defining each of the base colors (see not in testing section below)Methodology for Resolving Css Variables and Normalizing Colors
ThemeUtils.resolveCssVariablesInRecord(record, targetEl)
var
expressions from values in given recordvar
expression, create a tmp css variable on the temp divvar
expressions with resolved values. Values are resolved usinggetPropertyValue()
on the result ofgetComputedStyle()
in previous step.Testing
The MonacoTheme object now gets passed through a normalization function. You can see the before and after in the debug logs "Monaco theme:" and "Monaco theme derived:". The inputs are various
var(--dh-
expressions`, and the outputs should all be 8 character hex values.resolves #1542
BREAKING CHANGE: Theme variables have to be present on body to avoid Monaco init failing