-
Notifications
You must be signed in to change notification settings - Fork 0
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
Refactor pages #88
Refactor pages #88
Conversation
7a7fadb
to
543bdf4
Compare
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.
LGTM, don't care about point 1), but I would like to resolve point 2):
- I do not have a clear opinion if
Page.jsx
suffix was necessary, but I do not have problem with it on the other hand. I see that for Modal we have this convention (i.e.Modal.jsx
suffix), so ok. - I do not understand what is
DagrePage
and why it is in dagre directory. Dagre to me is the library that visualizes part of the screen, so I do not see a reason why to call it according to this technical aspect of the tool? (I understand it was not your idea) Moreover, I believe only part of the page is related to Dagre right? The page visualizes concrete script right?... If so I suggest usingdomain-specific name
instead. How about calling itScriptPage.jsx
.
To me, ScriptPage.jsx is better because it is more intuitive than Dagre. In future issues, we should refactor this component into separate ones, as the current 730-line component is very hard to understand... |
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.
LGTM
You are most-likely right but not sure where you got the convention Vocabulary.js --> vocabulary.js ...
Resolve #84