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

preliminary support for bool: [needs help] #347

Merged
merged 6 commits into from
Sep 9, 2024

Conversation

titoBouzout
Copy link
Contributor

The idea of the bool: namespace is to control if an attribute is present or not by evaluating its value to truthy/falsy. If it is truthy the attribute is added with an empty string as value, and if falsy the attribute is removed. It's similar to BooleanAttributes that dom-expressions already considers but controllable by the user. This is specially relevant in the web components world where names are arbitrary and cannot be known beforehand.

The namespace will make the intention clear and this cannot be done with dom-expressions setAttribute.

I am running into an issue trying to implement this feature and your help would be welcome

I have added 1 test for "custom elements"
babel-plugin-jsx-dom-expressions/test/__dom_fixtures__/customElements/code.js

When I run npm run test it displays two failing tests:

  • FAIL test/dynamic-dom.spec.js
  • FAIL test/dom.spec.js

It looks like It's reusing the test I wrote in two different places and expecting for the same test two different outputs, I can't figure out what's going on with this test. Any ideas?


if(!value) continue
prop = prop.slice(5);
result += `${escape(prop)}`;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

does this need a space?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ryansolid this has been merged, pinging because Im not sure if you saw the question above, thanks

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah it can but it depends which is why we have that logic after the condition. I'm going to slightly refactor to include it.

@titoBouzout titoBouzout marked this pull request as ready for review September 8, 2024 00:47
@titoBouzout
Copy link
Contributor Author

ok, I think this can be reviewed. The test I wrote works, there's still that failing auto-generated test that I don't know from where that's coming from.

@ryansolid ryansolid changed the base branch from main to next September 9, 2024 21:08
@ryansolid ryansolid merged commit 2208050 into ryansolid:next Sep 9, 2024
1 check failed
@ryansolid
Copy link
Owner

ryansolid commented Sep 9, 2024

If you are curious why the tests fail its because we have some modes of universal rendering which are hybrid. Ie.. they combine both custom rendering and dom renderer. The problem was that your custom element my-el wasn't registered as a dom element so it assumed it was a universal element and didn't get the right treatment. I was able to add them to the test config and it all worked.

@titoBouzout
Copy link
Contributor Author

Ah, I see, I will take a look at that if I run into this problem again, thank you!

ryansolid pushed a commit that referenced this pull request Sep 9, 2024
* preliminary support for ``bool:`

* make code smaller

* value of attribute should be empty when adding it to the dom

* inline bool: attribute into the template when possible

* fix prop name

* undo space test
@ryansolid
Copy link
Owner

@titoBouzout one question I had on further review is why set the it as an attribute instead of a prop. I guess we don't know if all booleans are reflective which is fair. I suppose if it was sufficient people could have just used prop:booleanThing. It is just we handle all other booleans as an properties. It's a minor thing but I'm always looking at size savings.

@titoBouzout
Copy link
Contributor Author

@ryansolid I think the main issue this solves is getting rid of the heuristic and being able to be explicit about the intention in a way that's simple and idiomatic to solid, complementary to attr: and prop:

Considerations... WC authors will do things in different ways, ones would expect a prop and others will expect an attribute. This could be relevant to the attributeChangedCallback. The tricky part is to think that external web components can be used in Solid, so one needs to imagine that they haven't used solid-element and the like, a bunch of solid ecosystem assumptions go out of the window. Giving control back empowers and I consider it important to not cause friction in adoption.

That being said, I am honestly still trying to figure out how all play together. I see a lot of benefices using WC and I'm trying to move to them, there are challenges. Joe expands on this feature in solidjs/solid#2158 and lit-html has similar features to prop/bool (they default to attribute I think).

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.

2 participants