P5.js-web-editor: Refactor IDEView

[catarak]

Nature of issue?

• Existing feature enhancement

Feature enhancement details:

In order to work on making the web editor mobile friendly, @ghalestrilo suggested refactoring and breaking apart IDEView.jsx. I, personally, have wanted to refactor this component for a while, so I think now is a great time to work on this.

[catarak]

Suggestions from @ghalestrilo:

Behavior

• move method getTitle to pure ext. function receiving props obj
• move method isUserOwner to pure ext. function receiving props obj

• move warnIfUnsavedChanges to pure ext. function which:

  • receives props obj
  • returns a callback (which receives route) (because of how it will be passed to the child component)
  • note: this function does too much: it should be further split

• move method handleGlobalKeydown to:

  • ext. function receiving props obj
  • ext. function receiving callbacks obj (derived from props)
  • redux action

Rendering

• Lines 210 to 246: move to functional component <Layout />

• Lines 335 to 376: create functional component <PreviewWrapper /> and wrap <PreviewFrame /> inside it:

<PreviewWrapper>
  <IDEView />
</PreviewWrapper>

• Lines 382 to 462: depending on how many prop passes are required
  
  
  
  • move to functional component <IDEOverlays />
  
  
  • move to connected component <IDEOverlays />
  
  
  • Create provider-like wrapper <WithOverlays /> and wrap the <IDEView /> inside it:
  
  

<WithOverlays>
  <IDEView />
</WithOverlays>

Lifecycle

• (optional) move code from lifecycle methods to ext functions (ensuring they're correctly bound to this)
• refactor component into hooks

"I broke it down into 3 parts to be a bit more manageable. And just getting the Behavior section will already make the migration a lot easier, cause it contains most of the core functionality. The rest is good but less important."

[ghalestrilo]

Analyzing/grouping the props received by <IDEView /> seems helpful to understand what responsibilities are being coupled in this view. I hope this can bring some insight for the refactor

// Modal Props: Could be provided through an overlay component
const modalProps = {
  toast: PropTypes.shape({
    isVisible: PropTypes.bool.isRequired
  }).isRequired,

  showErrorModal: PropTypes.func.isRequired,
  showEditorOptions: PropTypes.func.isRequired,
  showKeyboardShortcutModal: PropTypes.func.isRequired,
  showRuntimeErrorWarning: PropTypes.func.isRequired,
  hideRuntimeErrorWarning: PropTypes.func.isRequired,
  openUploadFileModal: PropTypes.func.isRequired,

  hideErrorModal: PropTypes.func.isRequired,
  closeUploadFileModal: PropTypes.func.isRequired,
  closeNewFolderModal: PropTypes.func.isRequired,
  closeNewFileModal: PropTypes.func.isRequired,
  closeShareModal: PropTypes.func.isRequired,
  closeEditorOptions: PropTypes.func.isRequired,
  closeKeyboardShortcutModal: PropTypes.func.isRequired,
};


// Project Props: Could be a context, or the <Preferences /> could be connected
const settingsProps = {
  closePreferences: PropTypes.func.isRequired,
  setFontSize: PropTypes.func.isRequired,
  setAutosave: PropTypes.func.isRequired,
  setLineNumbers: PropTypes.func.isRequired,
  setLinewrap: PropTypes.func.isRequired,
  setLintWarning: PropTypes.func.isRequired,
  setTextOutput: PropTypes.func.isRequired,
  setGridOutput: PropTypes.func.isRequired,
  setSoundOutput: PropTypes.func.isRequired,
  setAllAccessibleOutput: PropTypes.func.isRequired,
  setTheme: PropTypes.func.isRequired,

  preferences: PropTypes.shape({
    fontSize: PropTypes.number.isRequired,
    autosave: PropTypes.bool.isRequired,
    linewrap: PropTypes.bool.isRequired,
    lineNumbers: PropTypes.bool.isRequired,
    lintWarning: PropTypes.bool.isRequired,
    textOutput: PropTypes.bool.isRequired,
    gridOutput: PropTypes.bool.isRequired,
    soundOutput: PropTypes.bool.isRequired,
    theme: PropTypes.string.isRequired,
    autorefresh: PropTypes.bool.isRequired
  }).isRequired,
};

const layoutProps = {
  expandSidebar: PropTypes.func.isRequired,
  collapseSidebar: PropTypes.func.isRequired,
};

const consoleProps = {
  console: PropTypes.arrayOf(PropTypes.shape({
    method: PropTypes.string.isRequired,
    args: PropTypes.arrayOf(PropTypes.string)
  })).isRequired,
  clearConsole: PropTypes.func.isRequired,
  expandConsole: PropTypes.func.isRequired,
  collapseConsole: PropTypes.func.isRequired,
  dispatchConsoleEvent: PropTypes.func.isRequired,
};

// Project Props: Could be a context, or the <Sidebar /> could be connected
const projectProps = {
  project: PropTypes.shape({
    id: PropTypes.string,
    name: PropTypes.string.isRequired,
    owner: PropTypes.shape({
      username: PropTypes.string,
      id: PropTypes.string
    }),
    updatedAt: PropTypes.string
  }).isRequired,

  files: PropTypes.arrayOf(PropTypes.shape({
    id: PropTypes.string.isRequired,
    name: PropTypes.string.isRequired,
    content: PropTypes.string.isRequired
  })).isRequired,
  newFile: PropTypes.func.isRequired,
  deleteFile: PropTypes.func.isRequired,
  newFolder: PropTypes.func.isRequired,
  createFolder: PropTypes.func.isRequired,
  cloneProject: PropTypes.func.isRequired,

  updateFileName: PropTypes.func.isRequired,
  getProject: PropTypes.func.isRequired,
  saveProject: PropTypes.func.isRequired,
  autosaveProject: PropTypes.func.isRequired,
  setSelectedFile: PropTypes.func.isRequired,
  selectedFile: PropTypes.shape({
    id: PropTypes.string.isRequired,
    content: PropTypes.string.isRequired,
    name: PropTypes.string.isRequired
  }).isRequired,

  // These look like internal Sidebar state
  openProjectOptions: PropTypes.func.isRequired,
  closeProjectOptions: PropTypes.func.isRequired,
};

const sketchProps = {
  startSketch: PropTypes.func.isRequired,
  stopSketch: PropTypes.func.isRequired,
  endSketchRefresh: PropTypes.func.isRequired,
  startRefreshSketch: PropTypes.func.isRequired,
  // Sketch
  htmlFile: PropTypes.shape({
    id: PropTypes.string.isRequired,
    name: PropTypes.string.isRequired,
    content: PropTypes.string.isRequired
  }).isRequired,
}

IDEMobileView.propTypes = {
  ...modalProps,
  ...settingsProps,
  ...consoleProps,
  ...sketchProps,

  // Editor props
  ide: PropTypes.shape({
    isPlaying: PropTypes.bool.isRequired,
    isAccessibleOutputPlaying: PropTypes.bool.isRequired,
    consoleEvent: PropTypes.array,
    modalIsVisible: PropTypes.bool.isRequired,
    sidebarIsExpanded: PropTypes.bool.isRequired,
    consoleIsExpanded: PropTypes.bool.isRequired,
    preferencesIsVisible: PropTypes.bool.isRequired,
    projectOptionsVisible: PropTypes.bool.isRequired,
    newFolderModalVisible: PropTypes.bool.isRequired,
    shareModalVisible: PropTypes.bool.isRequired,
    shareModalProjectId: PropTypes.string.isRequired,
    shareModalProjectName: PropTypes.string.isRequired,
    shareModalProjectUsername: PropTypes.string.isRequired,
    editorOptionsVisible: PropTypes.bool.isRequired,
    keyboardShortcutVisible: PropTypes.bool.isRequired,
    unsavedChanges: PropTypes.bool.isRequired,
    infiniteLoop: PropTypes.bool.isRequired,
    previewIsRefreshing: PropTypes.bool.isRequired,
    infiniteLoopMessage: PropTypes.string.isRequired,
    projectSavedTime: PropTypes.string,
    previousPath: PropTypes.string.isRequired,
    justOpenedProject: PropTypes.bool.isRequired,
    errorType: PropTypes.string,
    runtimeErrorWarningVisible: PropTypes.bool.isRequired,
    uploadFileModalVisible: PropTypes.bool.isRequired
  }).isRequired,
  editorAccessibility: PropTypes.shape({
    lintMessages: PropTypes.array.isRequired,
  }).isRequired,
  updateLintMessage: PropTypes.func.isRequired,
  clearLintMessage: PropTypes.func.isRequired,
  updateFileContent: PropTypes.func.isRequired,
  router: PropTypes.shape({
    setRouteLeaveHook: PropTypes.func
  }).isRequired,

  // Context/Permission props
  params: PropTypes.shape({
    project_id: PropTypes.string,
    username: PropTypes.string,
    reset_password_token: PropTypes.string,
  }).isRequired,
  user: PropTypes.shape({
    authenticated: PropTypes.bool.isRequired,
    id: PropTypes.string,
    username: PropTypes.string
  }).isRequired,

  // Navigation props
  location: PropTypes.shape({
    pathname: PropTypes.string
  }).isRequired,
  route: PropTypes.oneOfType([PropTypes.object, PropTypes.element]).isRequired,
  setUnsavedChanges: PropTypes.func.isRequired,
  clearPersistedState: PropTypes.func.isRequired,
  persistState: PropTypes.func.isRequired,
  setBlobUrl: PropTypes.func.isRequired,
  setPreviousPath: PropTypes.func.isRequired,
};

[catarak]

Other ideas I have to add to this:

• Create a UserContext to handle authentication and authorization needs, such as isUserOwner (the current sketch could be passed to the UserContext).
• Use connect from react-redux in Editor, PreviewFrame, etc. to pass down fewer props
• Move most, if not all of the state in reducers/ide.js to React state. It will need to be split up in different components, where it makes sense.

[catarak]

• Move Preferences to own component, with connect
• Move Overlays to own component, with connect
• Preferences could be a context, maybe?
• Layout (panes layout, overlays) should be in React internal state (in IDEView?)

[ghalestrilo]

About the last point: I think of two approaches:

• Making <Layout /> could its own component, controlling modals within its state (so you'd wrap <IDEView />) with it
• Making <Overlays /> its own component, and replacing the current overlay code (inside <IDEView />) with it (passing props as necessary)

This really just depends on the level of coupling 🤷🏻 but it feels like the second option is somewhat simpler

Also, connecting <Editor /> and <PreviewFrame /> would cut down a ton on prop-passing, and I don't really think it harms incapsulation (might be wrong here)