-
Notifications
You must be signed in to change notification settings - Fork 35
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
Lox showcase (Fixed Commits) #197
base: main
Are you sure you want to change the base?
Conversation
Needed to create a new branch to fix the Eclipse ECA. So here is the new PR. |
@emilkrebs just a heads up. You need to rebase this once #196 is merged. But adjusting will not be hard. |
* Fix typo in getting-started grammar * Make code blocks vertically scrollable To make it possible to read the docs on a phone
* update of langium + monaco tutorial * update generation in the web tutorial
* Update to latest monaco-editor-wrapper and monaco-editor-react * Implemented review comments
* fixed nested elements and scaling * remaned one function * fixed a renaming problem * Fix typo in getting-started grammar (#198) * Fix typo in getting-started grammar * Make code blocks vertically scrollable To make it possible to read the docs on a phone * Update Langium + Monaco Tutorials (#192) * update of langium + monaco tutorial * update generation in the web tutorial * Update to latest monaco-editor-wrapper and monaco-editor-react (#196) * Update to latest monaco-editor-wrapper and monaco-editor-react * Implemented review comments * fixed a renaming problem --------- Co-authored-by: Vit Gottwald <[email protected]> Co-authored-by: Benjamin Friedman Wilson <[email protected]> Co-authored-by: Kai Salmen <[email protected]>
// get the size of the tree | ||
const getChildSize = (child: TreeNode): number => { | ||
// get the leaf nodes of the tree | ||
const getLeafNodes = (child: TreeNode): number => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getLeafCount
, right?! It is returning a number.
return 1 + Math.max(...child.children.map(getLongestPath)); | ||
}; | ||
|
||
const height = getLeafNodes(data) * 60; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A minor thing: You use magic numbers. Maybe give them a name. Especially when one number relates to the amount of another number. I guess these numbers relate to the max label size of the nodes or?
If there is no relation between these numbers and they are unique as they are used here, I think you can skip this comment of mine.
It is about code quality. Those hidden relationships can cost time for the next guy working on this file.
} | ||
|
||
// create a TreeNode from any DomainModelElement | ||
const getTreeNode = (e: DomainModelElement): TreeNode => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can remove the comment by giving proper names for getTreeNode
. Just name it getDomainModelTreeNode
. Same for the comments above. If the comment gives no extra information to the next line, it is basically useless and a danger of becoming outdated when the code changes again.
Comments are good for stuff that is not written in code (like motivations or hidden knowledge).
For me the website does not build. There is a problem with
Please make it also work for local environment, because I have no access using Gitpod. I guess we miss an entry in the Ah, I saw the description of you, sorry! |
Please rebase on the latest main branch to support PR previews (#210). |
Two steps are needed first in order to publish this