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

Allow editing of defined DOM properties #35

Closed
wants to merge 20 commits into from

Conversation

fluvf
Copy link
Contributor

@fluvf fluvf commented Dec 27, 2018

Ok, take number 23!
Now only changes element property handling and adds tests for it.

Crel now prioritizes element properties, if they're already defined, over adding / modifying attributes. This way you can edit properties like style or innerHTML, etc. using Crel, without having to use attrMap
This does remove the ability to use some reserved keywords in attributes, if you can't be certain that they are undefined, but I think that's bad practice, so we shouldn't encourage it anyways

Also updated the README to reflect these changes

@fluvf
Copy link
Contributor Author

fluvf commented Dec 27, 2018

I'm working on tests next, and will add some for this there. But if you want, I can also add them into this pull request

@fluvf
Copy link
Contributor Author

fluvf commented Dec 31, 2018

Currently this doesn't ignore null as the readme suggest, but I'll fix that in a follow up pr (or tomorrow)

@KoryNunn
Copy link
Owner

KoryNunn commented Jan 2, 2019

I would not like to allow the properties object in any other position than the second argument, it increases the surface area for testing, does not add any usability, and would probably decrease understandability.

Also, to implement this, there are multiple additional duck-typing checks per child added. This would likely have a notable impact on performance.

Copy link
Owner

@KoryNunn KoryNunn left a comment

Choose a reason for hiding this comment

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

I think I'm missing some tests, as these changes should fail tests. I'll have a look.

crel.js Outdated Show resolved Hide resolved
crel.js Show resolved Hide resolved
@KoryNunn
Copy link
Owner

KoryNunn commented Jan 2, 2019

These changes should fail this test: https://github.com/KoryNunn/crel/blob/master/test/index.js#L149

But there are too many conflicts for me to easily check this.

@fluvf
Copy link
Contributor Author

fluvf commented Jan 2, 2019

I would not like to allow the properties object in any other position than the second argument, it increases the surface area for testing, does not add any usability, and would probably decrease understandability.

Also, to implement this, there are multiple additional duck-typing checks per child added. This would likely have a notable impact on performance.

Yeah, my original use case for that became obsolete, so I'll look into it after a rebase

@fluvf
Copy link
Contributor Author

fluvf commented Jan 2, 2019

Rebased! No longer allows for multiple attributes objects to be passed. I also tried to make the changes a bit more incremental

crel.js Outdated Show resolved Hide resolved
crel.js Outdated Show resolved Hide resolved
} else if (isType(attribute, fn) || element[key]) {
if (isType(attribute, obj)) {
// We only check and allow for one level of object depth
for (value in attribute) {
Copy link
Owner

Choose a reason for hiding this comment

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

This looks like a helper for things like the style object? This is not really the intent of crel which aims to be extremely simple and raw. This is something that could easily be implemented in a more feature-rich module that uses crel under the covers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Isn't crel there to allow creating and editing ALL of the basic attributes and properties of an element? That's why I added the properties manipulation anyways. And with that in place it becomes kind of impossible to edit the style of an element through crel, because you can't access the attribute anymore.
On a side note, it's not specifically a helper for the style property, but then again I don't know if any other property is nested like that.

README.md Outdated Show resolved Hide resolved
@fluvf fluvf force-pushed the prioritize-props branch 2 times, most recently from 3d5b2e1 to 81b94b6 Compare January 23, 2019 12:25
@fluvf fluvf changed the title Rework argument handling and allow editing of defined DOM properties Allow editing of defined DOM properties Jan 23, 2019
@fluvf
Copy link
Contributor Author

fluvf commented Jan 23, 2019

Rebased. I split this into multiple prs. Currently based on #42, only adds the property changes. I'll open another pr for the invalid argument handling at some point

@fluvf fluvf force-pushed the prioritize-props branch 2 times, most recently from ee1a493 to d86452a Compare January 27, 2019 01:00
@fluvf
Copy link
Contributor Author

fluvf commented Feb 26, 2019

final rebase
I don't need / see a use for the changes in this pr anymore (surprise surprise writing better code removes arbitrary use cases) so feel free to close this 👍

@KoryNunn
Copy link
Owner

No problems. Would you be able to let me know which of the other PRs I should look at and in which order?

@KoryNunn KoryNunn closed this Feb 26, 2019
@fluvf
Copy link
Contributor Author

fluvf commented Feb 26, 2019

#46 and #47 (which is based on #46) in that order, thanks!

@fluvf fluvf deleted the prioritize-props branch February 26, 2019 23:29
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.

None yet

2 participants