Skip to content

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

codewithvk
Copy link
Collaborator

@codewithvk codewithvk commented Apr 5, 2025

  • Resolves: #
  • Target version: main

Summary

TODO

  • ...

Checklist

  • Code is properly formatted
  • Sign-off message is added to all commits
  • Documentation (manuals or wiki) has been updated or is not required

@codewithvk codewithvk force-pushed the private/codewithvk/pass_ui_theme_to_new branch from 136938b to 7eec888 Compare April 10, 2025 06:02
@codewithvk codewithvk changed the title Private/codewithvk/pass UI theme to new feat(setting): add dark/light iframe CSS vars Apr 10, 2025
@codewithvk codewithvk marked this pull request as ready for review April 10, 2025 06:02
@codewithvk codewithvk force-pushed the private/codewithvk/pass_ui_theme_to_new branch from 7eec888 to a729d0f Compare April 11, 2025 06:33
return result
}

const generateSettingCssVarTokens = () => {
Copy link
Member

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

Copy link
Collaborator Author

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:

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

Copy link
Contributor

@elzody elzody May 7, 2025

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.

@codewithvk codewithvk force-pushed the private/codewithvk/pass_ui_theme_to_new branch from a729d0f to 39741e1 Compare April 28, 2025 11:02
@codewithvk codewithvk requested a review from juliusknorr April 28, 2025 11:03
@codewithvk codewithvk force-pushed the private/codewithvk/pass_ui_theme_to_new branch from 39741e1 to 694fc6a Compare April 29, 2025 15:51
@elzody
Copy link
Contributor

elzody commented May 23, 2025

@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.

@codewithvk codewithvk force-pushed the private/codewithvk/pass_ui_theme_to_new branch from 694fc6a to 45c207d Compare May 26, 2025 08:52
@codewithvk
Copy link
Collaborator Author

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 :)

@juliusknorr juliusknorr force-pushed the private/codewithvk/pass_ui_theme_to_new branch from 45c207d to d0016e3 Compare May 27, 2025 06:21
Signed-off-by: codewithvk <vivek.javiya@collabora.com>
@codewithvk codewithvk force-pushed the private/codewithvk/pass_ui_theme_to_new branch from d0016e3 to 1dfe15d Compare May 27, 2025 07:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants