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

Feat/3208 backwards arrow direction #4019

Draft
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

brvv
Copy link

@brvv brvv commented Jan 21, 2023

📑 Summary

Add backward arrows to flowchart

Resolves #3208

This pr is not ready to merge yet, and I wanted to confirm the syntax/direction with the maintainers before I add documentation and e2e tests

📏 Design Decisions

For backward arrow syntax, I've decided to go with a separator \ that marks a link as being reversed:

A1 -\-> B1 --> C1

image

I chose this syntax because using <-- creates an inconsistency with the existing double arrow syntax. For example,

A1 --> B1 --> C1 // existing syntax that creates three nodes
A2 <-- B2 --> C2 // existing syntax to create two nodes and set 'B2' as a label. 

If backward arrow is implemented as <--, from a user's perspective, in cases like A2 <-- B2 --> C2, I would expect it to create three nodes, similar to A1 --> B1 --> C1. I've provided more details about this in the #3208, fell free to check it out!

Is the syntax that I chose ok for this issue? Alternatively, I can try to keep the <-- syntax, and prioritize the existing behaviour when possible for backward compatibility. For example,

A1 <-- B --> A2 // B is text

A1 <-- B
B --> A2 // B is a vertex

Thanks!

📋 Tasks

Make sure you

  • 📖 have read the contribution guidelines
  • 💻 have added unit/e2e tests (if appropriate)
  • 📓 have added documentation (if appropriate)
  • 🔖 targeted develop branch

@WolverineFan
Copy link

Not sure these are a ton better, but some other options:

  • -<-
  • <<--
  • <*-- (or some other non-dash character after <)

@jgreywolf jgreywolf added Type: Bug / Error Something isn't working or is incorrect Graph: Flow Status: In progress and removed feature labels Jan 26, 2023
@daniporr
Copy link

Any news on this?

@ioggstream
Copy link

Since <--- already produces a link, I think it would be better to ensure that the framework supports both link starts+end

@jgreywolf jgreywolf requested a review from sidharthv96 August 28, 2023 12:52
@jgreywolf
Copy link
Contributor

@brvv What is the status on this?

@sidharthv96
Copy link
Member

The implementation is very neat :)
But, I'm not so sure about the syntax. @knsv @aloisklink @nirname @Yokozuna59 any thoughts?

@nirname
Copy link
Contributor

nirname commented Sep 16, 2023

@sidharthv96 technically syntax is ok (speaking in terms of regexp and grammar), but I am not sure about its readability

@nirname nirname requested review from nirname and a team December 10, 2023 22:34
@nirname
Copy link
Contributor

nirname commented Dec 10, 2023

Seems we need to revive this one

@SpocWeb
Copy link

SpocWeb commented Mar 21, 2024

Since <--- already produces a link, I think it would be better to ensure that the framework supports both link starts+end

But this Syntax A <--- B currently ignores the < Character which cannot be the writer's intention:

graph LR
 A <--- B
Loading

so I think this intuitive Syntax is free to be re-purposed for a backward-pointing Link.

@sidharthv96
Copy link
Member

so I think this intuitive Syntax is free to be re-purposed for a backward-pointing Link.

That would have been wonderful, but the < character will be interpreted by the browser as an HTML tag opening.
So it will not work when mermaid diagrams are added directly in the html, like this.

<pre class="mermaid">
graph 
A <-- B
</pre>

@SpocWeb
Copy link

SpocWeb commented Mar 22, 2024

That would have been wonderful, but the < character will be interpreted by the browser as an HTML tag opening. So it will not work when mermaid diagrams are added directly in the html, like this.

Is that a valid use-case?

When embedding Text in (X)HTML not only the < has to be escaped using & lt; , but also at least > and & itself using & amp;

@sidharthv96
Copy link
Member

I'm afraid that it's a valid use case.

When embedding Text in (X)HTML not only the < has to be escaped using & lt; , but also at least > and & itself using & amp;

It's not necessary, we have many tests without any escaping.

image image

@ExeVirus
Copy link

ExeVirus commented Oct 16, 2024

Seems like some good suggestions for syntax have been made over in #3208, (direction specification being the only competing standout option) @brvv do you have time to rework this PR to use one of them and get it over the finish line?

@jgreywolf
Copy link
Contributor

As a side point, you can also reverse the order the nodes appear in. B -- edge --> A, vs A <-- edge -- B
This does not work as well if you are chaining nodes though

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Graph: Flow Status: In progress Type: Bug / Error Something isn't working or is incorrect
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Backwards arrow direction
9 participants