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

Refactor pages #88

Merged
merged 6 commits into from
Nov 13, 2024
Merged

Refactor pages #88

merged 6 commits into from
Nov 13, 2024

Conversation

palagdan
Copy link
Collaborator

Resolve #84

@palagdan palagdan requested a review from blcham November 12, 2024 14:52
@palagdan palagdan requested a review from blcham November 12, 2024 15:07
Copy link
Contributor

@blcham blcham left a 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):

  1. 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.
  2. 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 using domain-specific name instead. How about calling it ScriptPage.jsx.

@palagdan
Copy link
Collaborator Author

palagdan commented Nov 12, 2024

@blcham

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...

Copy link
Contributor

@blcham blcham left a 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 ...

@blcham blcham merged commit f7a501d into master Nov 13, 2024
2 checks passed
@blcham blcham deleted the 84-refactor-pages branch November 13, 2024 17:11
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.

Refactor page components into a pages folder and implement user-friendly NotFoundPage
2 participants