-
Notifications
You must be signed in to change notification settings - Fork 1
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
Reorganize frontend folder structure, nomeclature #266
base: main
Are you sure you want to change the base?
Conversation
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 few recommended changes, a few relevant comments, and some less relevant ones...
Overall looks good. I've pulled and run locally and everything still seemed to be present & loaded fine, as far as I could tell.
- **Areas** (`gui/src/app/areas`) are subdivisions of a page _with a_ unique | ||
_purpose_. An area can contain other areas which serve distinct sub-purposes, | ||
or one or more Panels. |
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.
- **Areas** (`gui/src/app/areas`) are subdivisions of a page _with a_ unique | |
_purpose_. An area can contain other areas which serve distinct sub-purposes, | |
or one or more Panels. | |
- **Areas** (`gui/src/app/areas`) are subdivisions of a page with a _unique | |
purpose_. An Area can contain other Areas which serve distinct sub-purposes, | |
or one or more Panels. |
- **Windows** (`gui/src/app/windows`) are Areas that use `CloseableDialogue` | ||
to pop in and out rather than occupying real estate of the Page. | ||
|
||
- **Panels** are subdivisions of an area. Sibling panels should all be pushing |
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.
- **Panels** are subdivisions of an area. Sibling panels should all be pushing | |
- **Panels** are subdivisions of an area. Sibling Panels should all be pushing |
to pop in and out rather than occupying real estate of the Page. | ||
|
||
- **Panels** are subdivisions of an area. Sibling panels should all be pushing | ||
"in the same direction". |
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.
I'm not sure I understand "pushing in the same direction." Do you mean serving the same broader purpose? I originally understood it to mean something about the orientation of the sliders.
For example, the `SamplerOutputArea` area contains tabs for different views of the output. | ||
Each of these tabs is one Panel -- while the draws as a table and the draws as | ||
histograms are quite different from each other, they serve the same greater purpose |
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.
For example, the `SamplerOutputArea` area contains tabs for different views of the output. | |
Each of these tabs is one Panel -- while the draws as a table and the draws as | |
histograms are quite different from each other, they serve the same greater purpose | |
For example, the `SamplerOutputArea` Area contains tabs for different views of the output. | |
Each of these tabs is one Panel -- while the Panel that displays the draws as a table is | |
quite different from the Panel that displays the draws as histograms, | |
they serve the same greater purpose |
For example, the `SamplerOutputArea` area contains tabs for different views of the output. | ||
Each of these tabs is one Panel -- while the draws as a table and the draws as | ||
histograms are quite different from each other, they serve the same greater purpose | ||
of allowing the user to explore the result of the Stan run, which is their parent Areas |
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.
of allowing the user to explore the result of the Stan run, which is their parent Areas | |
of allowing the user to explore the result of the Stan run, which is their parent Area's |
@@ -53,7 +59,7 @@ const StanFileEditor: FunctionComponent<Props> = ({ | |||
}); | |||
|
|||
// invalid syntax | |||
if (!validSyntax && !!editedFileContent) { | |||
if (!validSyntax && !!data.ephemera.stanFileContent) { |
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.
Not exactly germane to this PR, but my fingers are itching to move the button stuff to its own file, or at least find a way to make it less repetitive.
import { ToolBar, ToolbarItem } from "@SpComponents/ToolBar"; | ||
import { UserSettingsContext } from "@SpSettings/UserSettings"; | ||
import { CodeMarker } from "@SpStanc/Linting"; | ||
import { UserSettingsContext } from "@SpCore/Settings/UserSettings"; |
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.
Didn't you say in the architecture doc that components shouldn't interact with contexts? (It is probably the right thing to do here, just noting it)
@@ -1,5 +1,5 @@ | |||
import { createContext } from "react"; | |||
import { SettingsTab } from "./SettingsWindow"; | |||
import { SettingsTab } from "@SpWindows/SettingsWindow"; |
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.
import { SettingsTab } from "@SpWindows/SettingsWindow"; | |
import { type SettingsTab } from "@SpWindows/SettingsWindow"; |
@@ -11,7 +11,7 @@ import { | |||
|
|||
import { useDialogControls } from "@SpComponents/CloseableDialog"; | |||
|
|||
import { SettingsTab } from "./SettingsWindow"; | |||
import { SettingsTab } from "@SpWindows/SettingsWindow"; |
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.
import { SettingsTab } from "@SpWindows/SettingsWindow"; | |
import { type SettingsTab } from "@SpWindows/SettingsWindow"; |
@@ -59,7 +69,22 @@ const TopBar: FunctionComponent<TopBarProps> = ({ title, onSetCollapsed }) => { | |||
<Brightness7 fontSize="inherit" /> | |||
)} | |||
</IconButton> | |||
<CompilationServerConnectionControl /> | |||
<IconButton |
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.
Inlining this component (from CompilationServerConnectionControl) is not unreasonable, it isn't super big, but it's somewhat less semantic.
This closes #113. This just moves code around and renames some things, no behavioral changes
The organizing principles are defined in a new ARCHITECTURE.md document: link
src/app
now has the following top-level folders:pages
-- just HomePageareas
-- sub-views of HomePage. The majority of the UI is here and its sub-folderswindows
-- modal boxes that show up above HomePage, like the settingscomponents
-- reusable componentscore
-- contexts and workers, non-UI codeutil
-- utility functionsFor easier viewing, here's the folder layout in full:
Details