-
Notifications
You must be signed in to change notification settings - Fork 0
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 all content nodes to be referenced #101
base: master
Are you sure you want to change the base?
Conversation
case p: (String, Node, Node) if p._2.data.isInstanceOf[NodeData.Placeholder] => | ||
new SearchSourceEntry { | ||
title = p._2.id.toCuidString | ||
placeholder = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need to store this placeholder var separately? It is already encoded in the data property (that is the node itself).
We have talked about this shortly. I understand the use-case but I am honestly not convinced that this is the right approach. It could solve some problems and enable some use-case, but I think it does so in a difficult to manage way. It is difficult to track which property reference resolves to which property. It might be ok in automations, but might become rather cumbersome to manage for multiple properties. What happens if properties are exchanged or deleted? I fear that the user has to do all that work by himself and that it becomes therefore difficult to understand. |
I think it is okay that the user has to do the manual work then as of now and that maintaining it may be cumbersome. However, this could be improved later. |
It is like with pointer - maybe we can make them smart in the future |
you got me there...programming language also started smart pointers later down the road. okee. |
@@ -650,7 +651,7 @@ object Components { | |||
def searchAndSelectNodeApplied[F[_] : Sink : Source](current: F[Option[NodeId]], filter: Node => Boolean)(implicit ctx: Ctx.Owner): VNode = searchAndSelectNode(current, filter) --> current | |||
def searchAndSelectNode[F[_] : Source](observable: F[Option[NodeId]], filter: Node => Boolean)(implicit ctx: Ctx.Owner): EmitterBuilder[Option[NodeId], VNode] = | |||
Components.searchInGraph(GlobalState.rawGraph, "Search", filter = { | |||
case n: Node.Content => InlineList.contains[NodeRole](NodeRole.Message, NodeRole.Task, NodeRole.Project)(n.role) && filter(n) | |||
case n: Node.Content => filter(n) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did you check whether other usages depends on this?
} | ||
source = { | ||
val g: Graph = graph.now | ||
val res1 = (g.nodes.collect { case node: Node if filter(node) && node.role == NodeRole.Neutral => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about other noderoles than neutral?
@@ -169,6 +169,9 @@ trait SearchSourceEntry extends js.Object { | |||
var description: js.UndefOr[String] = js.undefined | |||
var category: js.UndefOr[String] = js.undefined | |||
|
|||
var placeholder: js.UndefOr[Boolean] = js.undefined |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not really belong into the fomantic ui facade, as it is our state, right?
7face5a
to
e37a4fc
Compare
@cornerman so vllt für den anfang?