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

remarkSyntaxDiagram: fix rendering of (x y z)* #145

Merged
merged 2 commits into from
Jan 20, 2021

Conversation

kennytm
Copy link
Contributor

@kennytm kennytm commented Jan 19, 2021

In an ENBF+diagram, components like (x y z)* is currently rendered like this:

>>--->---------------------v---><
    (                       )
     ^--[ x ]-[ y ]-[ z ]--<

This is wrong as the lower part is traveled from right to left, so those following the railroad precisely will get the order z y x instead.

This PR changes the rendering to something like this instead to keep the LTR order.

       >---------------------------v
      /                             \
>>---^--->---[ x ]-[ y ]-[ z ]---v--->---><
        (                         )
         ^-----------------------<

Single-node-wide components like x* are still rendered with the existing layout.

@g1eny0ung g1eny0ung self-assigned this Jan 20, 2021
@g1eny0ung g1eny0ung self-requested a review January 20, 2021 03:55
@g1eny0ung
Copy link
Contributor

BTW, I think we can implement OneOrMore and ZeroOrMore separately. Because the railroad-diagrams library can render ZeroOrMore out of the box.

@kennytm
Copy link
Contributor Author

kennytm commented Jan 20, 2021

@g1eny0ung railroad-diagrams's ZeroOrMore(x) is equivalent to Optional(OneOrMore(x)), so I don't bother adding another type

@g1eny0ung
Copy link
Contributor

g1eny0ung commented Jan 20, 2021

@g1eny0ung railroad-diagrams's ZeroOrMore(x) is equivalent to Optional(OneOrMore(x)), so I don't bother adding another type

The current implementation is OK. I'm just thinking about is necessary to process single node x* and (x y z)* individually?

If someone else is involved in the development process, she/he may feel a little complicated. (They may think x* will also be ZeroOrMore, but we actually implement to OneOrMore)

@g1eny0ung
Copy link
Contributor

@g1eny0ung railroad-diagrams's ZeroOrMore(x) is equivalent to Optional(OneOrMore(x)), so I don't bother adding another type

The current implementation is OK. I'm just thinking about is necessary to process single node x* and (x y z)* individually?

If someone else is involved in the development process, she/he may feel a little complicated. (They may think x* will also be ZeroOrMore, but we actually implement to OneOrMore)

🤣 My fault. I missed item: { type: 'Skip' } in:

return options.context.isRTLCapable(p) ?
  {type: 'OneOrMore', item: {type: 'Skip'}, repeat: p} :
  {type: 'Optional', item: {type: 'OneOrMore', item: p, repeat: {type: 'Skip'}}};

All LGTM!

@g1eny0ung g1eny0ung merged commit f5da074 into pingcap:master Jan 20, 2021
@kennytm kennytm deleted the prevent-rtl-stars branch January 20, 2021 08:19
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