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 all content nodes to be referenced #101

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

GRBurst
Copy link
Member

@GRBurst GRBurst commented Oct 20, 2019

@cornerman so vllt für den anfang?

referenceProperties

referencePropertiesPicking

case p: (String, Node, Node) if p._2.data.isInstanceOf[NodeData.Placeholder] =>
new SearchSourceEntry {
title = p._2.id.toCuidString
placeholder = true
Copy link
Member

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).

@cornerman
Copy link
Member

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.

@GRBurst
Copy link
Member Author

GRBurst commented Oct 20, 2019

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.

@GRBurst
Copy link
Member Author

GRBurst commented Oct 20, 2019

It is like with pointer - maybe we can make them smart in the future

@cornerman
Copy link
Member

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)
Copy link
Member

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 =>
Copy link
Member

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
Copy link
Member

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?

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.

2 participants