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

[Bug Fix] Wrapper will not print false attributes #5412

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

promatik
Copy link
Member

This is a fix for both;


WHY

BEFORE - What was wrong? What was happening before this PR?

Some links where empty:

image

AFTER - What is happening after this PR?

No more empty links.

image

HOW

How did you achieve that, in technical terms?

All wrapper_start files (for columns, fields and widgets) will check if the value of attribute is NOT false.

Basically if someone has a closure for an HREF, or any other attribute, that attribute will be always printed in the HTML Dom element, even if the value is false, or null.

With this PR, developers can control that by returning false on the closure, it's a strict validation so any falsy value will be printed anyway, 0, null (even though null is not really printed on the html).

null href=""
0 href="0"
"" href=""
false

Is it a breaking change?

I don't believe so.
This is only a breaking change for users who were returning false on some closure, and were expecting the attribute to be rendered empty.

How can we test the before & after?

By following @karandatwani92 suggestion here #5391 (comment)

@pxpm
Copy link
Contributor

pxpm commented Dec 30, 2023

I think this is a major BC 😢
Not only on the case you mentioned, but it would also prevent developers from adding ANY custom attribute with value false to the wrapper.

// ...
'wrapper' = > [
'customAttr' => false,
]

I will be merging the fix for linkTo() in a separate PR until we figure out if we can do this on a non-breaking way.

🙏

@promatik
Copy link
Member Author

promatik commented Jan 7, 2024

@pxpm as we discussed, this is now validation href only 👌

@promatik promatik assigned pxpm and unassigned promatik Jan 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

None yet

2 participants