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

change Node to Observable, link video #1307

Closed
wants to merge 3 commits into from

Conversation

Datseris
Copy link
Contributor

@Datseris Datseris commented Sep 9, 2021

Can someone tell me whether the way I've embedded html is correct...?

Can someone tell me whether the way I've embedded html is correct...?
@Datseris
Copy link
Contributor Author

Datseris commented Sep 9, 2021

cc @SimonDanisch this should be the start of getting rid of Node.

@Datseris
Copy link
Contributor Author

Datseris commented Sep 9, 2021

Okay, so now the question is: should I do a massive search and replace all Node -> Observable?

@ffreyer
Copy link
Collaborator

ffreyer commented Sep 9, 2021

If we're getting rid of Node should we also get rid of lift? Maybe I'm missing something but my impression is that it's the same as map/map!.

@Datseris
Copy link
Contributor Author

Datseris commented Sep 9, 2021

wait WHAT????? All this time I've been living in a LIE??? I thought they were different functions!!! Oh my god yes PLEASE let's remove all these duplicate names!!!!!!

@jkrumbiegel
Copy link
Member

The difference is that lift executes once I think, so it creates a typed observable, while map creates an untyped one. At least I think it used to be like that.

@ffreyer
Copy link
Collaborator

ffreyer commented Sep 10, 2021

They both do that now. (I checked with Observables 4.0 and Makie 0.15.2+ with integers)

From Makie:

function lift(f, o1::Observables.AbstractObservable, rest...; kw...)
    # compat...
    init = f(to_value(o1), to_value.(rest)...)
    typ = typeof(init)
    result = Observable{typ}(init)
    map!(f, result, o1, rest...)
    return result
end

From Observables:

@inline function Base.map(f::F, arg1::AbstractObservable, args...; kwargs...) where F
    # compat...
    map!(f, Observable(f(arg1[], map(to_value, args)...)), arg1, args...; update=false)
end

Those are the same except for update = false as far as I can tell.

By default observable gets updated immediately, but this can be suppressed by specifying update=false.

I guess that's be why lift triggers twice on creation, i.e. #1303?

@SimonDanisch
Copy link
Member

Seems like this broke the docs:

→ Initial full pass...
┌ Franklin Warning: in <nodes.md>
│ Encountered an issue processing 'nodes.md' in l/docs/documentation.
│ Verify, then re-start the Franklin server.
│ The error is displayed below:
│ Franklin.OCBlockError("I found the opening token 'CODE_TRIPLE' but not the corresponding closing token.", "Context:\n\t...fullscreen></iframe>\n```\n\n## The `Observabl... (near line 10)\n\t                        ^---\n")
└
ERROR: LoadError: I found the opening token 'CODE_TRIPLE' but not the corresponding closing token.
Context:
	...fullscreen></iframe>

The `Observabl... (near line 10)

                        ^---


## The `Node` structure
```@raw html
Copy link
Member

Choose a reason for hiding this comment

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

@jkrumbiegel do you know how to properly inline this with Franklin?

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 have seen you guys have already put the tutorial in the home page under tutorials, so maybe we can just crosslink there? (I don't know how)

Copy link
Member

Choose a reason for hiding this comment

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

Raw html goes between ~~~ fences

Copy link
Member

Choose a reason for hiding this comment

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

Cross-linking would also work, although there isn't the perfect thing to link to I guess

@SimonDanisch SimonDanisch mentioned this pull request Oct 19, 2021
@SimonDanisch
Copy link
Member

Merged in #1393

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.

4 participants