Skip to content

Conversation

@kennytm
Copy link
Contributor

@kennytm kennytm commented Aug 2, 2020

This PR introduced a remark plugin to supplement code blocks of the form:

```ebnf+diagram
TableName ::= ( Identifier '.' )? Identifier
```

with an SVG syntax diagram. A <SyntaxDiagram> shortcode is introduced for the user to toggle between the diagram and EBNF.

This is intended to gradually replace the entire media/sqlgram directory, to unify the appearances, simplify documentation process, and improve accessibility (images can't be searched).

Preview
Syntax diagram view
Source code view

The syntax diagram is generated using @prantlf/railroad-diagrams, and the style has been tweaked to mimic the Railroad Diagram generator used to produce the original images. There are two differences though:

  1. The diagrams do not "wrap". You have to scroll horizontally if they become too long.
  2. The drop shadow filter does not work on Chrome, so they are removed.
Horizontal scrollbar

3-fs8

In theory @prantlf/railroad-diagrams has the Stack element for manually breaking up the diagram into multiple rows. But in reality there is some bug in the height calculation which caused different rows to overlap. So in this PR we won't include such feature. (And scrolling is needed for mobile anyway.)

@kennytm
Copy link
Contributor Author

kennytm commented Oct 22, 2020

ping @YiniXu9506

@YiniXu9506
Copy link
Contributor

@kennytm Thanks for your contribution, I will take a look ASAP.

@g1eny0ung
Copy link
Contributor

@kennytm Sorry for so late to review this PR because I turned to develop the Chaos Mesh last several months.

I will dig into the implementation of this remark plugin in the next few days.

@kennytm
Copy link
Contributor Author

kennytm commented Nov 28, 2020

Thanks @g1eny0ung.

Material UI has been replaced by Bulma in e0a2773. The style is slightly adjusted (the buttons are no longer ALL CAPS, which was forced by MUI).

Preview

1

I recommend not to use @material-ui/core because our UI/UX designers want to use the @material-ui/icons. 😢

If we're using Material UI only for the icons, why not use the Material Design Icons instead, which is the same but has zero dependencies 🤔

MUI icons MDI icons
Add mdi-plus
Language mdi-web
Search mdi-magnify
Flag mdi-flag
AccountTreeRounded* mdi-sitemap, rotated left by 90°
Code* mdi-code-tags
Attachment mdi-attachment
Warning mdi-alert

(* = introduced in this PR, so can be replaced by other icons I like 🙃)

@g1eny0ung
Copy link
Contributor

g1eny0ung commented Nov 29, 2020

If we're using Material UI only for the icons, why not use the Material Design Icons instead, which is the same but has zero dependencies 🤔

After searching, I found that if we want to use this package with React. We still need to import the @mdi/react library to provide an Icon wrapper. In essence, the difference is not big.

Why we using the @material/icons at the beginning (at that time, we thought it met our needs, so we used it directly, and didn’t consider other options.), these are our three reasons:

  1. The @material/icons provide an effective tree shaking solution, so we don't need to import all icons.
  2. The Icon wrapper provides consistent behavior for all SVG images, so we can use the other custom images without care about something more.
  3. Full documentation support.

Copy link
Contributor

@g1eny0ung g1eny0ung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 🍾

@g1eny0ung g1eny0ung merged commit 4e586b4 into pingcap:master Nov 30, 2020
@g1eny0ung g1eny0ung added enhancement New feature or request feature A new feature need to support labels Nov 30, 2020
@lilin90
Copy link
Member

lilin90 commented Nov 30, 2020

@nullnotnil @lysu FYI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request feature A new feature need to support

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants