reflex-dev / reflex

🕸️ Web apps in pure Python 🐍
https://reflex.dev
Apache License 2.0
19.62k stars 1.12k forks source link

[REF-3167] `rx.select` does not propagate props #3541

Open abulvenz opened 3 months ago

abulvenz commented 3 months ago

Describe the bug Using e.g. id=... in rx.select does not show up in the rendered UI.

To Reproduce

Expected behavior All superficial props should be propagated without asking, right? We had the same problem once with rx.input and its wrapping in debounce.

Additional context Maybe related: https://github.com/reflex-dev/reflex/issues/1802

Low prio, use the above workaround.

REF-3167

benedikt-bartscher commented 3 months ago

Maybe it's a good idea to add a pytest fixture for all reflex components and test that the id prop (and maybe some other props) is propagated

tfc1209 commented 3 months ago

Use rx.chakra.select

abulvenz commented 3 months ago

Use rx.chakra.select

@tfc1209 Thanks for the tip. That would work. We are just trying to reduce our bundle size by removing all chakra elements :smile:

@benedikt-bartscher I like your idea. We had this problem already a few times, e.g. with the internal debounce-input-wrapping. My first thought was creating some automated test that adds ids and styles but there are more exceptions from the rule than otherway round. So I think the best is to write the components down explicitly in one app and maybe have something that tells us, when to add new components to it. As the implementer you can then decide if a new component goes on the ignore list (for the automatic detection) or to add a test-case for it. I will give it a try.

abulvenz commented 3 months ago

OK, I tried something here: https://github.com/abulvenz/reflex/blob/prop-propagation-testing/integration/test_prop_propagation.py

Maybe this is not the best way to do it. Anyway, the result is that we are at least also missing id-propagation on

rx.avatar
rx.data_editor
rx.moment
rx.editor
rx.select (already known)

The functions use the wcb i.e. wrap_component_broken method, which skips the retrieval by ID, because it would break the test. In the first draft I tried to do it with automatic component instanciation, but as I wrote there are many exceptions from the rule. https://github.com/abulvenz/reflex/commit/3897e14028496d5269460231248b709023485f48

Lendemor commented 3 months ago

Maybe it's a good idea to add a pytest fixture for all reflex components and test that the id prop (and maybe some other props) is propagated

Made an issue ( #3564) to track this idea, so it's not forgotten when we fix the specific issue with rx.select