-
Notifications
You must be signed in to change notification settings - Fork 151
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
Add dark mode compatibility to Twinkle #2023
base: master
Are you sure you want to change the base?
Conversation
P.S. If you make future pull requests, you should probably do it on a different branch in your repo than master. Can get buggy especially if you do multiple pull requests at the same time. |
color: var(--color-subtle, #54595d); | ||
} | ||
|
||
/* Override select2 silliness */ |
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.
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 we might need to fork select2, and make our own custom css styles for dark mode. Applying this css hotfix to morebits.css is only likely to cause problems down the road.
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.
Long term, I don't think we should even keep select2 (#1154).
For now, applying some hacks is enough. The reason these rules don't seem to apply is because the dropdown is created outside of .morebits-dialog-content
– the select2-container is a direct child of body
. Works fine with the following:
.select2-container,
.select2-results,
.select2-container--default .select2-results__option[aria-selected=true],
.select2-container--default .select2-selection__rendered,
.select2-container--default .select2-selection--multiple,
.select2-container--default .select2-selection__choice {
background: var(--background-color-base, #fff) !important;
color: inherit !important;
}
Tested on Chrome and Firefox.
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.
My main concern with this solution is because it's not specific to Twinkle, it may affect styles for content outside of Twinkle. That's why I suggested changing the select2 library's css directly.
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.
All select2 CSS we have today are added via JS in the module code (example) so that they are applied only when that Twinkle dialog is opened. We could do the same here, though it will still affect non-Twinkle content when these modules (warn/xfd/block) are opened. I think it's a reasonable trade-off.
There's no way around that short of creating a custom select2 theme, which involves duplicating a lot of CSS of the default theme.
Maintaining a fork of the entire library is not something we have bandwidth for. Even if we had, select2 fits in poorly with MediaWiki's interfaces and we should ideally move away from it (in favour of integrations with native libraries like Codex or OOUI), not deepen our involvement with it.
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 found a way to avoid impacting non-Twinkle content. Turns out we can use the theme
field to hack in an extra class on the select2-container. By setting theme: 'default select2-morebits'
, the default theme styles get applied, and select2-morebits
class also appears on the container using which we can target the styles:
.select2-morebits,
.select2-morebits .select2-results,
.select2-morebits .select2-results__option[aria-selected=true],
.select2-morebits .select2-selection__rendered,
.select2-morebits .select2-selection--multiple,
.select2-morebits .select2-selection__choice {
background: var(--background-color-base, #fff) !important;
color: inherit !important;
}
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'm not that familiar with select2, where do I apply the theme
field, so it applies to all select2 objects? I tried doing so by changing the default in lib/select2.min.js, but that didn't work.
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.
Actually I found a better strategy:
.morebits-dialog ~ .select2-container:first-of-type > .select2-dropdown {
background: var(--background-color-base, #fff) !important;
color: inherit !important;
}
This CSS only matches the first select2-container after morebits-dialog, therefore only matching select2 twinkle content.
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'm not that familiar with select2, where do I apply the
theme
field
In the .select2({...})
initialization at each place where select2 is used (in twinklewarn.js, twinkleblock.js and twinklexfd.js).
so it applies to all select2 objects?
Even if it was possible to apply it in one place, that would mean any non-Twinkle uses also get the styles, which is what you set out to avoid in the first place?
Actually I found a better strategy:
That's a pretty complex selector. I think using .select2-morebits
in place of .morebits-dialog ~ .select2-container:first-of-type
is cleaner and less error-prone.
--morebits-color-info: #6BDB6B; | ||
--morebits-color-warning: #FFC7B3; | ||
--morebits-color-titlebar-links: #8BADDF; | ||
--morebits-bgcolor-dialog: #17244A; |
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 is subjective, but I think the blue on black looks terrible. Might want to use a shade of grey instead.
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.
Sorry, which color are you talking about?
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.
morebits-bgcolor-dialog
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.
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 it's better to reuse --background-color-neutral
or --background-color-neutral-subtle
for the dialog background. Titlebar colour seems okay.
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.
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.
Choice of colors is dependent on the theme. Pale blue dialog against a white background looks nothing like this gawdy dark blue dialog on a black background. If the contrast with the titlebar is a problem, that can be changed as well.
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.
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.
Yes, this looks okay to me.
color: var(--color-subtle, #54595d); | ||
} | ||
|
||
/* Override select2 silliness */ |
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.
Long term, I don't think we should even keep select2 (#1154).
For now, applying some hacks is enough. The reason these rules don't seem to apply is because the dropdown is created outside of .morebits-dialog-content
– the select2-container is a direct child of body
. Works fine with the following:
.select2-container,
.select2-results,
.select2-container--default .select2-results__option[aria-selected=true],
.select2-container--default .select2-selection__rendered,
.select2-container--default .select2-selection--multiple,
.select2-container--default .select2-selection__choice {
background: var(--background-color-base, #fff) !important;
color: inherit !important;
}
Tested on Chrome and Firefox.
--morebits-color-info: #6BDB6B; | ||
--morebits-color-warning: #FFC7B3; | ||
--morebits-color-titlebar-links: #8BADDF; | ||
--morebits-bgcolor-dialog: #17244A; |
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.
morebits-bgcolor-dialog
@@ -216,7 +258,8 @@ div.morebits-usertext { | |||
|
|||
body .ui-dialog.morebits-dialog .ui-dialog-titlebar { |
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.
The close button is nearly invisible in dark mode. We might want to customise its colour as well.
I have tested locally, it seems to work.