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
HTML Refactor #2164
HTML Refactor #2164
Conversation
Alek99
commented
Nov 13, 2023
•
edited
edited
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.
This is great. I think documenting and maintaining this rx.el
namespace is going to be good for people who want a truly lightweight app. With no chakra/radix components in use, then they wont be installed at all!
potential conflict with this recent PR: e761a04
The rx.el.element.Element.render
method will still end up accidentally getting sx
props because they will get added in _render
and not removed here. I'm thinking that now is probably the time to have Component
pass style
as style
and have ChakraComponent
pass style
as sx
. Then we can stop special casing that here and in the radix tree.
reflex/el/elements/base.py
Outdated
"""Base class for common attributes.""" | ||
|
||
# Provides a hint for generating a keyboard shortcut for the current element. | ||
access_key: Var_[Union[str, int, bool]] |
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'd like to read this type as access_key: Var_[Attribute]
or maybe Var_[HTMLAttribute]
.
something that reduces the complexity of what my brain has to grok to understand the line.
reflex/el/elements/inline.py
Outdated
from .base import BaseHTML | ||
|
||
|
||
class A(BaseHTML): # Inherits common attributes from BaseMeta |
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.
class A(BaseHTML): # Inherits common attributes from BaseMeta | |
class A(BaseHTML): |
One question should I move the .el into the components directory? |
I think it would make more sense in there, but I think we should still expose it as |
…ut for deploy command, remove deploy legacy command (#2179)
* add in new no_of_lines prop for text * black update --------- Co-authored-by: Tom Gotsman <tomgotsman@toms-mbp.lan>
scripts/precompile.py
Outdated
@@ -14,7 +14,7 @@ | |||
|
|||
from reflex.utils import path_ops | |||
|
|||
from .constants import ELEMENT_TO_PROPS, ELEMENTS | |||
from ..reflex.components.el.constants import ELEMENT_TO_PROPS, ELEMENTS |
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 shouldn't need a relative import, does the normal from reflex.components.el.constants
work 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.
Looks awesome.
For future work, we still need to get event triggers defined on some of these, but I think we should roll with what is already a big improvement.
The Svg
element is essentially a separate XML doc embedded in the HTML, and I don't know how we want to handle that, but as it is, i don't think we will be reverse compiling svg to reflex code anytime soon, nor do i think that's desirable from a perf perspective. We can probably just inline the svg code in the Svg
element for the reverse compiler in the short term.
size: Text field size "1" - "3" | ||
size: Specifies the visible width of a text control |
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.
The inherited HTML element description probably shouldn't take precedent over the description from the class