pingcap / website-docs

The next generation of PingCAP Docs. Powered by Gatsby ⚛️.
https://docs.pingcap.com/
MIT License
22 stars 34 forks source link

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

Closed kennytm closed 3 years ago

kennytm commented 3 years ago

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 commented 3 years ago

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

kennytm commented 3 years ago

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

g1eny0ung commented 3 years ago

@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 commented 3 years ago

@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!