-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
report(redesign): dark theme #8425
Conversation
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!
do you mind rebasing/renaming the PR to remove the merge blockers? |
renamed. I don't understand how rebasing would remove the other two failures. The CLA? |
for whatever reason the Travis merge with master isn't picking up #8583, so rebase/merge with master would force it to include the fix |
my track record with rebasing is awful, but I think just merging master back in will do the trick. |
btw: git commit --allow-empty -m "empty commit to trigger CI" |
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.
looks nice. 🕶
* @param {KeyboardEvent} e | ||
*/ | ||
onKeyUp(e) { | ||
if (e.key === 'd') { |
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.
this seems fine for development, but I don't think we should leave it in.
either remove now that we're done with this PR or remove in a week?
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.
done
#8185
Merge base is a branch I made w/ #8222 and #8307.merged in master.Still need to do the PWA badge, and inline some svgs (it prevents duplicating by letting CSS change the colors. Only inlining svgs that would get used just once.)