Refactor title handling #5168

benjiwheeler · 2019-09-12T21:31:02Z

Resolves

Should resolve #3433 and scratchfoundation/scratch-desktop#71 ;

Note that it does not seem to resolve the related #4759 .

Proposed Changes

Repurpose TitledHOC so that it:

no longer maintains projectTitle state
no longer passes projectTitle down to children
no longer sets canEditTitle to true on children
has responsibility for setting redux projectTitle when parent passes a changed projectTitle prop (previously this functionality was in containers/gui.jsx)
notices when the project-state changes to isShowingWithoutId, uses the translated default project name to set the redux projectTitle, and also calls the parent'sonUpdateProjectTitle prop function

GUI's playground/standalone versions previously used TitledHOC, so they need to explicitly pass canEditTitle to children.

scratch-desktop will need to no longer wrap itself with TitledHOC, and instead maintain its own projectTitle state.

Reason for Changes

in standalone, project title persists after upload; isn't set to default when File->New #3433 and File>New makes leaves the name of the previous project in the title field scratch-desktop#71 were bugs
we want to localize the default project name

cwillisf

I feel a bit out of my depth but this seems to make sense to me. Thanks for working on this!

rschamp · 2019-09-25T20:24:26Z

src/containers/gui.jsx

    isLoading: PropTypes.bool,
    isScratchDesktop: PropTypes.bool,
    isShowingProject: PropTypes.bool,
+    isShowingWithoutId: PropTypes.bool,


Is this added intentionally? It looks unrelated.

Removing...

rschamp · 2019-09-25T20:27:44Z

src/lib/titled-hoc.jsx

+                this.props.onUpdateProjectTitle(defaultProjectTitle);
+            }
+        }
+        titleWithDefault (title) {


Why is this a function that takes a title argument? It looks like it's only called in one place, without an argument.

I think I meant to refactor setReduxTitle further... doing this now

rschamp · 2019-09-25T20:30:26Z

src/lib/titled-hoc.jsx

                <WrappedComponent
-                    canEditTitle
-                    projectTitle={this.state.projectTitle}
                    onUpdateProjectTitle={this.handleUpdateProjectTitle}


If we expect children to receive the title from redux, I think we should we also expect them to set it through redux.

I made this change, now testing it...

Tested well

benjiwheeler · 2019-10-11T20:21:29Z

src/lib/titled-hoc.jsx

+            if (this.props.projectTitle !== prevProps.projectTitle) {
+                this.props.updateReduxProjectTitle(this.titleWithDefault(this.props.projectTitle));
+            }
+            // if the projectTitle hasn't changed, but the reduxProjectTitle


Note this addition... here is where the change to redux title gets echoed up to the enclosing context (i.e., www)

benjiwheeler · 2019-10-18T15:10:32Z

src/components/menu-bar/project-title-input.jsx

    }
 });

-class ProjectTitleInput extends React.Component {


I should change this back to how it was before

I think this is fine to do since your handler is coming from Redux now. I just think the name of the prop should change.

Doing this...

benjiwheeler · 2019-10-18T15:14:13Z

src/lib/titled-hoc.jsx

-                projectTitle: null
-            };
+        componentDidMount () {
+            this.props.updateReduxProjectTitle(this.titleWithDefault(this.props.projectTitle));


can combine these calls

Changing...

rschamp

I have a few nits, but I think this is looking good overall. Especially that we don't need to consider projectId as a part of this.

rschamp · 2019-10-17T18:06:03Z

src/components/menu-bar/project-title-input.jsx


 ProjectTitleInput.propTypes = {
    className: PropTypes.string,
+    handleUpdateReduxProjectTitle: PropTypes.func,


I'm surprised the linter didn't catch this, because I think it used to. Prop names for events should be onX like onUpdateReduxProjectTitle. The name of the event handler itself should be handleX. The distinction is that the prop describes that the component might receive an event. The name of the handler describes that something should happen when that event is received.

What I'm used to the linter insisting on is that this form be followed:

onXXXX={this.handleXXXX}

In my experience it doesn't like

onXXXX={this.onXXXX}

but it's ok with

onXXXX={this.props.whateverNameIsOK}

rschamp · 2019-10-17T18:07:55Z

src/containers/sb-file-uploader.jsx

-    onUpdateProjectTitle: PropTypes.func,
    projectChanged: PropTypes.bool,
    requestProjectUpload: PropTypes.func,
+    updateReduxProjectTitle: PropTypes.func,


Same comment here about event props. onUpdateReduxProjectTitle would be more consistent.

Changing...

rschamp · 2019-10-18T15:36:04Z

src/components/menu-bar/project-title-input.jsx

    }
 });

-class ProjectTitleInput extends React.Component {


I think this is fine to do since your handler is coming from Redux now. I just think the name of the prop should change.

src/playground/player.jsx

…efault

rschamp

Thanks for all your work on this!

benjiwheeler added the pr - needs review label Sep 12, 2019

benjiwheeler added this to the September 2019 milestone Sep 12, 2019

benjiwheeler requested a review from cwillisf September 12, 2019 21:31

benjiwheeler assigned cwillisf Sep 12, 2019

benjiwheeler requested a review from rschamp September 15, 2019 00:38

benjiwheeler assigned rschamp Sep 15, 2019

benjiwheeler mentioned this pull request Sep 15, 2019

DesktopHOC manages project title, rather than wrapping with TitledHOC scratchfoundation/scratch-desktop#74

Merged

benjiwheeler added the needs discussion label Sep 15, 2019

cwillisf previously approved these changes Sep 16, 2019

View reviewed changes

benjiwheeler unassigned cwillisf Sep 23, 2019

rschamp suggested changes Sep 25, 2019

View reviewed changes

rschamp added pr - changes requested and removed needs discussion pr - needs review labels Sep 26, 2019

rschamp assigned benjiwheeler and unassigned rschamp Sep 26, 2019

benjiwheeler modified the milestones: September 2019, October 2019 Oct 1, 2019

benjiwheeler dismissed cwillisf’s stale review via 76b4c6f October 11, 2019 20:17

benjiwheeler force-pushed the clear-title-on-default-project branch from 117b940 to 76b4c6f Compare October 11, 2019 20:17

benjiwheeler assigned rschamp and unassigned benjiwheeler Oct 11, 2019

benjiwheeler added pr - needs review and removed pr - changes requested labels Oct 11, 2019

benjiwheeler force-pushed the clear-title-on-default-project branch from 76b4c6f to ced7ee7 Compare October 11, 2019 20:20

benjiwheeler commented Oct 11, 2019

View reviewed changes

benjiwheeler changed the title ~~on showing project without id, tell owner of GUI to set title to default~~ Refactor title handling Oct 16, 2019

benjiwheeler added 3 commits October 17, 2019 13:26

on showing project without id, tell owner of GUI to set title to default

91e2fc9

move project title management into titled hoc

b63ac51

remove TitledHOC from playground, and explicitly pass canEditTitle

9c33bbb

benjiwheeler added 2 commits October 17, 2019 13:26

don’t pass onUpdateProjectTitle below titledHIC

22e2c17

added back missing canEditTitle

68780ae

benjiwheeler force-pushed the clear-title-on-default-project branch from ced7ee7 to 68780ae Compare October 17, 2019 17:26

benjiwheeler commented Oct 18, 2019

View reviewed changes

benjiwheeler added the pr - changes requested label Oct 18, 2019

benjiwheeler self-assigned this Oct 18, 2019

rschamp suggested changes Oct 18, 2019

View reviewed changes

rschamp removed their assignment Oct 18, 2019

rschamp removed the pr - needs review label Oct 18, 2019

benjiwheeler added 2 commits October 18, 2019 13:40

better names for prop functions

b6ff1f4

in titledhoc, consolidate function calls

f4a9bfa

benjiwheeler assigned rschamp and unassigned benjiwheeler Oct 18, 2019

benjiwheeler added pr - needs review and removed pr - changes requested labels Oct 18, 2019

revised name of updateReduxProjectTitle, updateReduxProjectTitleWithD…

e3fa196

…efault

rschamp approved these changes Oct 21, 2019

View reviewed changes

rschamp added pr - good to merge and removed pr - needs review labels Oct 21, 2019

rschamp assigned benjiwheeler and unassigned rschamp Oct 21, 2019

benjiwheeler merged commit 395b558 into scratchfoundation:develop Oct 21, 2019

benjiwheeler deleted the clear-title-on-default-project branch October 21, 2019 14:52

benjiwheeler mentioned this pull request Oct 31, 2019

in standalone, project title persists after upload; isn't set to default when File->New #3433

Closed

benjiwheeler mentioned this pull request Nov 12, 2019

reset title on transitioning to isShowingWithoutId #5291

Merged

benjiwheeler mentioned this pull request Feb 24, 2020

Revert "Revert "refactor SB* file uploader into top-level HOC"" #5450

Merged

Refactor title handling #5168

Refactor title handling #5168

Uh oh!

Conversation

benjiwheeler commented Sep 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Resolves

Proposed Changes

Related

Reason for Changes

Uh oh!

cwillisf left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rschamp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rschamp left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

benjiwheeler commented Sep 12, 2019 •

edited

Loading