Skip to content
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

[REF-2587] Ignore top-level theme appearance #3119

Merged
merged 2 commits into from
Apr 19, 2024

Conversation

masenf
Copy link
Collaborator

@masenf masenf commented Apr 19, 2024

From the Radix docs, it is not recommended to actually set appearance, but instead use next-themes to set and switch the appearance dynamically.

Because Reflex already compiles the top-level theme into the next-themes ThemeProvider, we can blank out the appearance prop after compiling contexts.js to avoid a mismatch between the selected app appearance and the appearance in the rx.theme when displaying overlay components.

Fix #2992


Bonus: fix the pyi_generator with pre-commit to stop creating .pyi files for those in the EXCLUDED_FILES list


Sample code

import reflex as rx


def dialogme():
    return rx.dialog.root(
        rx.dialog.trigger(rx.button("Dialog")),
        rx.dialog.content(
            rx.dialog.title("Foo"),
            rx.dialog.description("This is a dialog"),
            rx.dialog.close(
                rx.button("Close"),
            ),
        ),
    )


def index() -> rx.Component:
    return rx.vstack(
        rx.color_mode.button(rx.color_mode.icon()),
        dialogme(),
        rx.theme(
            dialogme(),
            appearance="dark",
        ),
        rx.theme(
            dialogme(),
            appearance="light",
        ),
    )


app = rx.App(theme=rx.theme(appearance="dark"))
app.add_page(index)

When running pyi_generator in pre-commit, it passes individual changed files on
the command line as targets, and these were not being properly excluded
according to the EXCLUDED_FILES list.

Add app.py to the EXCLUDED_FILES list so it does not get regenerated
automatically.
From the Radix docs, it is not recommended to actually set `appearance`, but
instead use next-themes to set and switch the appearance dynamically.

Because Reflex already compiles the top-level theme into the next-themes
ThemeProvider, we can blank out the appearance prop after compiling contexts.js
to avoid a mismatch between the selected app appearance and the appearance in
the rx.theme when displaying overlay components.

Fix #2992
Copy link

linear bot commented Apr 19, 2024

@masenf masenf merged commit 6ca5f48 into main Apr 19, 2024
46 checks passed
masenf added a commit that referenced this pull request Apr 22, 2024
* pyi_generator: ignore `app.py` and single files

When running pyi_generator in pre-commit, it passes individual changed files on
the command line as targets, and these were not being properly excluded
according to the EXCLUDED_FILES list.

Add app.py to the EXCLUDED_FILES list so it does not get regenerated
automatically.

* [REF-2587] Ignore top-level theme appearance

From the Radix docs, it is not recommended to actually set `appearance`, but
instead use next-themes to set and switch the appearance dynamically.

Because Reflex already compiles the top-level theme into the next-themes
ThemeProvider, we can blank out the appearance prop after compiling contexts.js
to avoid a mismatch between the selected app appearance and the appearance in
the rx.theme when displaying overlay components.

Fix #2992
@masenf masenf deleted the masenf/discard-radix-themes-appearance branch April 24, 2024 18:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants