-
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
rearrange main layout #102
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 couple minor things, but we can also just revisit later.
const LowerRightView: FunctionComponent<LowerRightViewProps> = ({ width, height, compiledMainJsUrl, dataFileContent, dataIsSaved, samplingOpts, setSamplingOpts }) => { | ||
const LowerRightView: FunctionComponent<LowerRightViewProps> = ({ width, height, compiledMainJsUrl }) => { | ||
const { data, update } = useContext(SPAnalysisContext) | ||
const dataIsSaved = data.dataFileContent === data.ephemera.dataFileContent |
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 see this as a definition internal to the data model, & think it'd probably be better to keep this internal to that file & expose the check via a function (though that function would probably just be const dataIsSaved = (data: SPAnalysisDataModel) => (data.dataFileContent === data.ephemera.dataFileContent)
)
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 have added the function and have named it modelHasUnsavedDataFileChanges to go along with the existing modelHasUnsavedChanges. I thought "dataIsSaved" sounded too generic.
const RightView: FunctionComponent<RightViewProps> = ({ width, height, compiledMainJsUrl }) => { | ||
return ( | ||
<LowerRightView | ||
width={width} | ||
height={height} | ||
compiledMainJsUrl={compiledMainJsUrl} | ||
/> | ||
) | ||
} |
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.
Seems a little weird to have a RightView
component whose sole content is the LowerRightView
component. Are we planning to expand this in the future?
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.
Oops that was a remnant of the prior layout. Removed the extra component.
Moved data editor to left panel, underneath the stan editor, to make more room for the sampling controls and sampling output panel on the right. Made some other minor adjustments of spacing and sizing. Simplified the data flow in the HomePage. Replaces #45