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

Seventh composite pull request #86

Merged
merged 10 commits into from Mar 6, 2021
Merged

Seventh composite pull request #86

merged 10 commits into from Mar 6, 2021

Conversation

hmdne
Copy link
Member

@hmdne hmdne commented Feb 10, 2021

Of the things that I want to document, maybe this should first come to mind:

d = DOM { input(type: "text") }
d["required"] = true # Worked
d["required"] = false # Had an unintuitive behavior before
d.attributes.delete("required") # Alternative syntax

# Also:
req = false
d = DOM { input(type: "text", required: req) } # This works now
# Used to require this before:
d = DOM {
  h = {type: "text"}
  h.merge!(required: "") if req
  input(h)
}

In general I develop opal-browser while developing an application, so those features come as what I miss while developing it.

@hmdne hmdne force-pushed the master branch 3 times, most recently from ce80aa6 to 19a1063 Compare February 12, 2021 02:47
Copy link
Member

@elia elia left a comment

Choose a reason for hiding this comment

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

Just one thing on how to improve an each implementation, otherwise 👍

Comment on lines 175 to 181
hash = {}
%x{
for (var pair of #@native.entries()) {
#{hash[`pair[0]`] = `pair[1]`}
}
}
hash.each(&block)
Copy link
Member

Choose a reason for hiding this comment

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

Since we're iterating we could just yield the two pairs without creating a supporting hash, that way we'd also get the advantage of supporting duplicate keys (which are a thing IIRC)

@elia elia merged commit 5ce4863 into opal:master Mar 6, 2021
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