-
Notifications
You must be signed in to change notification settings - Fork 122
feat(setting): add dark/light iframe CSS vars #4657
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?
feat(setting): add dark/light iframe CSS vars #4657
Conversation
136938b
to
7eec888
Compare
7eec888
to
a729d0f
Compare
src/helpers/coolParameters.js
Outdated
return result | ||
} | ||
|
||
const generateSettingCssVarTokens = () => { |
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.
Can't we just reuse the iexisting css vars? Feels a bit like duplicating quite some logic
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 are two points to consider:
- Missing Variables: Some variables are not currently being sent, so we need to add them.
- Theme-Based Values: For the settings iframe, we expect the variable values to be set based on the theme. (See the code pointer here: https://github.com/nextcloud/richdocuments/pull/4657/files#diff-087176fb56b5928e33a834558791bd88511f1897d4fd950a38b7334fe9c56c99R78-R82 .)
Since for cool iframe, we're not sending dark mode values(https://github.com/codewithvk/richdocuments/blob/private/codewithvk/pass_ui_theme_to_new/src/helpers/coolParameters.js#L105), I think creating new pair would make sense here... Any thought? @juliusknorr
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.
If I understood you correctly, maybe then we could still use the generateCSSVarTokens()
method and pass in some object containing values to override or something, and adjust the logic inside. That way we can use the same method and if you need to explicitly control the CSS variables (for in this case, the settings) then you can explicitly pass them to this method and it would mix them in, overriding preexisting ones.
I also don't want it to be too confusing to reason about, so two methods could probably be suitable, but it might help with re-usability or something.
This comment was marked as spam.
This comment was marked as spam.
a729d0f
to
39741e1
Compare
39741e1
to
694fc6a
Compare
@codewithvk Have not tested, but seems fine, thanks for that. Just needs the merge conflict resolved. Was going to try helping out with it myself but with the fork, not sure how it would work. |
694fc6a
to
45c207d
Compare
Sorry @juliusknorr & @elzody, I was busy with other stuff and couldn’t manage to fix it earlier. Anyway, I’ve modified generateCSSVarTokens and come up with a better approach — setting the iframe from Cool is now expected to use theme-based variables. If the theme is light, the variable values are based on light, and vice versa. Feel free to let me know if you have any questions :) |
45c207d
to
d0016e3
Compare
Signed-off-by: codewithvk <vivek.javiya@collabora.com>
d0016e3
to
1dfe15d
Compare
Summary
TODO
Checklist