Skip to content

Add ignore-nullish modifier to x-html and x-text #4579

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

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

Conversation

willrowe
Copy link
Contributor

@willrowe willrowe commented Mar 17, 2025

This adds an ignore-nullish modifier to x-html and x-text. If the value is set to null or undefined, the currently set HTML/text will remain unchanged.

The main use case for this is to have the initial HTML/text set in the markup on page load. See the updates to the documentation for examples.

@ekwoka
Copy link
Contributor

ekwoka commented Mar 18, 2025

I like the modifier approach.

I think I'd be more partial to preserve-nullish as opposed to ignore but that's just my opinion.

initTree(el)
delete el._x_ignoreSelf
})
if (!modifiers.includes('ignore-nullish') || value !== null) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (!modifiers.includes('ignore-nullish') || value !== null) {
if (!modifiers.includes('ignore-nullish') || value == null) {

Then you can remove the above value = value ?? null

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rather explicitly coerce to null and be able to do a strict check. It makes it more clear what the intent is here.

initTree(el)
delete el._x_ignoreSelf
})
if (!modifiers.includes('ignore-nullish') || value !== null) {
Copy link
Contributor

@ekwoka ekwoka Mar 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (!modifiers.includes('ignore-nullish') || value !== null) {
if (!modifiers.includes('ignore-nullish') || value != null) {

NIT: Then you can remove the above value = value ?? null

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rather explicitly coerce to null and be able to do a strict check. It makes it more clear what the intent is here.

Comment on lines +9 to +11
value = value ?? null;

if (!modifiers.includes('ignore-nullish') || value !== null) {
Copy link
Contributor

@ekwoka ekwoka Mar 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
value = value ?? null;
if (!modifiers.includes('ignore-nullish') || value !== null) {
if (!modifiers.includes('ignore-nullish') || value != null) {

NIT

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rather explicitly coerce to null and be able to do a strict check. It makes it more clear what the intent is here.

@willrowe
Copy link
Contributor Author

willrowe commented Mar 18, 2025

@ekwoka

I think I'd be more partial to preserve-nullish as opposed to ignore but that's just my opinion.

preserve-nullish implies that you are preserving a nullish value, which I would find confusing.

@ekwoka
Copy link
Contributor

ekwoka commented Mar 18, 2025

Yeah....but preserve-on-nullish is too long 😭

@Tim-Wils
Copy link

What is the value becomes null again after being set to a value? Shouldn't the value become the original innerHTML again with this modifier?

@willrowe
Copy link
Contributor Author

@Tim-Wils no, as the name infers, it ignores any nullish values and does not change the HTML/text. It will be most useful on initial state.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants