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

Add boolean version of setAttr #240

Merged
merged 3 commits into from
Mar 23, 2023
Merged

Conversation

ryukoposting
Copy link
Contributor

Related issue: #239

This PR adds a new setAttr proc, which accepts a boolean value instead of a string. When the value is true, it's the same thing as doing vnode.setAttr("key", val = ""). When the value is false, it iterates through VNode.attrs and deletes the attribute, if it exists.

The net effect of this new proc is that it is much easier to deal with attributes that need to be dynamically added/removed from the VDOM.

Example:

proc submitButton(dataIsValid: bool): VNode =
  buildHtml(tdiv):
    button(disabled = not dataIsValid):
      if dataIsValid:
        text "Submit"
      else:
        text "Cannot submit, data is invalid!"

@geotre
Copy link
Collaborator

geotre commented Mar 2, 2023

@ryukoposting did you have any luck with the methods described in #239? My concern with this approach is that it will be slower as it must iterate through the attributes, whereas the existing toChecked/toDisabled/toSelected do not need to. What do you think?

@ryukoposting
Copy link
Contributor Author

ryukoposting commented Mar 8, 2023

@ryukoposting did you have any luck with the methods described in #239? My concern with this approach is that it will be slower as it must iterate through the attributes, whereas the existing toChecked/toDisabled/toSelected do not need to. What do you think?

Apologies for the delay- got busy with work/personal things again. The performance hit associated with iterating through a very short list is miniscule compared to the overhead that comes with every VDOM re-draw. If you can perceive a difference, then sure. I like my solution because it is more generalized- i can set any kind of boolean attribute, whether it's selected, contenteditable, or notdefinedbythedombutcouldbeusedforsomething.

geotre
geotre previously requested changes Mar 9, 2023
@geotre
Copy link
Collaborator

geotre commented Mar 9, 2023

After some more thought, I think this will work well if we also use the same method as the existing helpers (when on js backend). See suggested change above @ryukoposting

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Co-authored-by: geotre <georgetrevill@gmail.com>
@geotre geotre dismissed their stale review March 15, 2023 13:58

Changes implemented

@geotre
Copy link
Collaborator

geotre commented Mar 15, 2023

This looks great now @ryukoposting, good job adding documentation also.

@Araq I think this is ready to go if you want to merge

@geotre
Copy link
Collaborator

geotre commented Mar 23, 2023

Ok I'm going to go ahead and merge this so I can create a new release. Araq let me know if you want me to handle things differently in the future

@geotre geotre merged commit 6609c32 into karaxnim:master Mar 23, 2023
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