-
-
Notifications
You must be signed in to change notification settings - Fork 66
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
refactor: change fill alias from "as var" to default=var #504
refactor: change fill alias from "as var" to default=var #504
Conversation
@@ -422,12 +422,12 @@ def render(self, context: Context) -> str: | |||
f"Detected duplicate fill tag name '{resolved_name}'." | |||
) | |||
|
|||
resolved_fill_alias = fill_node.resolve_alias(context, resolved_component_name) |
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 renamed the internal variables from alias
to slot_default_var
and from scope
to slot_data_var
, so it's clear what they refer to.
|
||
|
||
class UserSlotVar: | ||
class LazySlot: |
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.
Renamed and changed so that the bound slot + context is rendered as {{ slot }}
instead of {{ slot.default }}
.
if len(bits) >= 2 and bits[-2].lower() == "as": | ||
alias = bits.pop() | ||
bits.pop() # Remove the "as" keyword | ||
alias_fexp = parser.compile_filter(alias) |
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.
Seeing how we checked for the as var
syntax, I'm glad that we now use a kwarg to set this value, so that the behavior of the tags is more standardized.
Housekeeping note:
There are other inputs to various tags (slot
/component
) where use custom logic to parse the inputs. E.g. component's only
and slot's required
or default
. 3-4 features down the line, depending on how things go, we could reach a point where also these could be parsed as kwargs (e.g. required
would be inferred as required=True
). In such case, we could further simplify the logic for parsing tag inputs.
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 many other Django tags that use valueless attributes, so I'm not that bothered about this complexity personally.
@@ -912,7 +912,7 @@ def test_both_slots_overridden(self): | |||
self.assertHTMLEqual(rendered, '<p id="a">Override A</p><p id="b">Override B</p>') | |||
|
|||
|
|||
class SlotSuperTests(BaseTestCase): |
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.
Renamed the slot super
to slot default
@@ -998,6 +1000,52 @@ def test_super_under_if_node(self): | |||
""", | |||
) | |||
|
|||
def test_nested_fills(self): |
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.
Added a test to ensure the variable assigned via default
is accessible also in nested components. And did the same for the data
attribute too.
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 great! Since this is a breaking change, I think we should have a warning at the top of the README about this changed behavior.
@EmilStenstrom Thanks for the review! Good catch with the breaking change, I've added a note. |
Change the API for accessing slot's default content from
to
Closes #502