-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Mediaquery filter #9216
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: master
Are you sure you want to change the base?
Mediaquery filter #9216
Conversation
add mediaquery filter operator
use mediaquery filter operator to determine which palette to use
use mediaquery filter operator in base vanilla stylesheet to determine the color-scheme
✅ Deploy Preview for tiddlywiki-previews ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Confirmed: BurningTreeC has already signed the Contributor License Agreement (see contributing.md) |
📊 Build Size Comparison:
|
Branch | Size |
---|---|
Base (master) | 2537.6 KB |
PR | 2541.2 KB |
Diff: ⬆️ Increase: +3.6 KB
@@ -4,7 +4,7 @@ tags: $:/tags/Macro | |||
<!-- Needs to stay that way for backwards compatibility. See GH issue: #8326 --> | |||
\define colour(name) | |||
\whitespace trim | |||
<$transclude tiddler={{$:/palette}} index="$name$"> | |||
<$transclude tiddler={{{ [[$:/palettedark]mediaquery[(prefers-color-scheme: dark)]get[text]] :else[[$:/palette]get[text]] }}} index="$name$"> |
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.
What are the braces used for? (prefers-color-scheme: dark)
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.
That's just how a media query looks like. Should they be removed? 😊
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 did read the MDN docs and saw the Parameters docs paragraph. It says the braces are needed ...
But in TW filter syntax, they may cause confusion. So I am not sure anymore.
For the time being, I think we should keep it as it is atm. In the docs we will need to make sure, that the braces belong to the query and not to the filter syntax. ...
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.
We can remove the braces but then we need to add them programmatically
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.
MDN says every media-feature must be wrapped into parentheses. So it could be added by the filter -- But I am not really sure about that one. -- What if users search the web and see, that parentheses are needed, so they add them. We will end up with 2 of them. IMO for the time being it should be as it is. (Just my gut feeling)
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.
Ok yes I think you're right that leaving the ()
is the best
Do you know the draft PR: #8702 from Jeremy? |
core/modules/filters/mediaquery.js
Outdated
|
||
// Only evaluate in browser environment | ||
if($tw.browser && mediaQuery && widget) { | ||
try { |
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 is the if() statement, which should catch all problems. We need to make sure, that the variables are initialised. So why do we still need a try/catch statement here.
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.
As far as I can see. window.matchMedia()
does not throw any exceptions
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'll remove the try catch
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.
Wait, the try catch is indeed needed. Invalid media queries throw exceptions.
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.
are you sure the window.matchMedia()
is throwing or is it something else?
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.
Let me test, I'm not quite sure
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.
You're right @pmario , it doesn't throw exceptions. I've removed the try/catch statement
Yes I vaguely remember. But this is more about the filter operator than about palettes. I just added them as an example |
core/modules/filters/mediaquery.js
Outdated
|
||
// Add the listener (use modern addEventListener if available) | ||
if(mql.addEventListener) { | ||
mql.addEventListener("change", changeHandler); |
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 just saw this. I am not sure, if you can add an event-handler to a filter operator
@Jermolene - Is that possible?
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.
It's possible in terms of it's doable
😁 but if it's allowed in the core - that's a different question.
But this event listener would make this filter operator special
because it doesn't react to tiddler changes but to media-query changes
As @pmario mentioned and after some tests, this should not throw errors
This PR adds a
mediaquery
filter operator to TiddlyWiki5Example use:
In this PR it's used to alter the
<<colour>>
macro and to determine thecolor-scheme
on the:root
element like above