Project configuration #120

AWolf81 · 2018-08-06T12:02:08Z

Related Issue: #28

Summary:

Adds an animated gear icon to ProjectPage
Opens a configuration modal for:
- Project name
- Project icon
Adds a temporary project.middleware - needs to be converted to Redux-Saga later after PR Queue/group actions that lock package.json #103

This branch is based on npm-to-yarn.

Todos:

Add Gear button to project pane to open modal (with hover effect scale, color & rotate)
Create ProjectModal
Add icon picker from CreateNewProject Wizard
Create basic settings modal
Check flow types
Remove not needed code (copied from TaskModal & not needed here)
Add gear rotation on hover
Used Redux-based modal (maybe needs to be improved later - at dynamic modal component based on state.modal)
Add Save
Cancel button - maybe later
Add business logic - Rename / change everything (similar to when the project is created):
- Update guppy.name inside package.json
- Use slug to get an alphanumeric+dash version of the new name, and set that to guppy.id inside package.json, as well as package.json's name field.
- Rename the parent folder.
- Refresh state to use the new project settings
- Using uuid - no id renaming required
Display confirm for renaming at every project rename
Improve styling of project name text input & buttons
Check if package.json requires locking - wait for Queue and batch dependency actions #181
Create temporary middleware for renaming & modifying project folder & package.json - later change to Saga
Rework modal handling as mentioned in the issue either CurrentModal component or adding ProjectConfigurationModal after CreateProjectWizard to app.js - I need to check which is easier to maintain and more re-usable
Change Redux Action to single SAVE_PROJECT_SETTINGS action & remove Flow types for Redux --> check if we should create an issue for this.
Make ProjectIconSelection Component - for now code copied from CreateProjectWizard Will be refactored in a separate issue later
```
  <ProjectIconSelection
      selectedIcon={?string}
      showRandomSubset={boolean}
      onSelectIcon={function} />
```
Check initial loading? Maybe icons need a loading indication. Seems fast enough. Check directly after first app start - also looks OK.
Optimize renaming for imported projects & avoid brief flash of component with-out values (see second screenrecording)
Fix problem with paths.reducer - Path not found after clicking Nope in confirmation dialog during renaming an imported project. Also paths state object is always empty. It should work if we're getting guppy.path from package.json into paths state. Not sure what's wrong here. Update: Seems to be fixed after merging master & updating to Saga.
Check if SAVE_PROJECT_SETTINGS_START in project.middleware can be simplified. It's working but looks pretty complicated. Update: Simplified with re-write to a Saga.
Remove temporary project-save.middleware
Convert middleware to a Saga.
Add test to save-project-settings.saga
Test error handling e.g. rename or delete project externally or change permissions of directory.

Screen recording (gear & modal) - current version

Confirm for imported project

…into windows-support modified: scripts/start.js modified: src/middlewares/task.middleware.js

superhawk610 · 2018-08-13T19:30:57Z

Since the gear wheel is a button, I would probably add cursor: pointer to indicate as much. The animation looks really nice 👍

joshwcomeau

Excited to see this come together!

Unfortunately I do have a few things I'd like to see changed. The biggest is probably not changing the ID when the project name changes. We should still prompt the user to rename the folder, but I think changing the ID introduces a lot of complexity and potential issues, without having a clear benefit for the user.

Another thing I noticed is that this PR is still in a WIP state in some ways - lots of commented-out code, and debugging statements. I find it really helpful to review my own code when I open a PR, before adding reviewers, to help me catch this stuff before it gets to other reviewers. I initially added comments for stuff like this, but there's quite a few of them and I didn't want the more-important comments to get lost in the noise.

(FWIW I forget to do this, so I'm far from perfect myself haha. Just a good tip to try and remember)

Finally, it'd be great if you could try and create smaller PRs in the future. This can be pretty tough, but it makes it much easier to review. Often I'll create multiple PRs (choosing previous branches in the sequence as the target, instead of master), so that even not-fully-formed-features can have their own PR

Let me know if any feedback is unclear or shortsighted, happy to discuss!

joshwcomeau · 2018-08-18T14:38:19Z

src/components/SettingsButton/SettingsButton.js

+                  360}deg) scale(${scale}, ${scale})`,
+                color: chroma
+                  .interpolate(this.props.color, this.props.hoverColor, color)
+                  .hex(),


Interesting, I read through the chroma-js API docs and couldn't find an interpolate method o_O

I'm actually not sure we need to do this. My big hesitation is that color-changes will require a CPU repaint on every frame (whereas transform can happen entirely on the video card), so it might make the overall animation more stuttery on lower-end machines. I'm also a bit wary of adding another color library just for this (we already have NPM color, although Chroma looks like a better lib so I'd be up for switching... just have to update the existing color stuff).

Finally, I've been thinking about switching Guppy to use React Spring (https://github.com/drcmda/react-spring). It has a nicer API than React Motion, and comes with a few extra bells & whistles, like color interpolation built-in :o

So maybe for now we can omit the hover color change entirely, and consider re-adding it when we switch? I'd actually like to do the React Spring switch sooner rather than later, since it's a friendlier API. I might tackle that myself in the coming days.

Ah, forgot about this!

I think for now we should just set the color without animation:

color: hovered ? this.props.hoverColor : this.props.color

I'm concerned about the performance of the rotation animation if we're also mutating the color, for reasons described above. And I think a hard switch probably looks fine :)

joshwcomeau · 2018-09-04T16:28:18Z

package.json

@@ -37,6 +37,7 @@
    ]
  },
  "dependencies": {
+    "chroma-js": "^1.3.7",


Can we remove the caret, and put this in devDependencies instead of dependencies?

The reason is to avoid having stuff in node_modules with the shipped app, for anything compiled anyway by Webpack.

I added this to #210

OK, I'll move it and remove the caret.

I think once this PR is merge we could check if the chroma is really needed as I think the effect is almost not visible. It's blending the color of the gear icon.
But maybe we can use the blending elsewhere as I think this can be a nice effect if it is more visible.

joshwcomeau · 2018-09-04T16:29:57Z

src/actions/index.js

@@ -13,7 +13,7 @@ import type {

 //
 //
-// Action Types
+


I think we still want this? Doesn't seem super important, but it should stay consistent with the Action creators comment below.

Mostly I like this as a way to separate out files that have different kinds of things, like this. It's helpful in reducers, for example, which can also have selectors and helpers.

If other people don't think they're necessary, we can remove it :) but then we should also remove // Action creators below.

Oh, this is probably removed accidentially. I'll add it back.
Not sure how this happened.

joshwcomeau · 2018-09-04T16:33:37Z

src/components/App/App.js

+    selectedProject: getSelectedProject(state),
+    isAppLoaded: getAppLoaded(state),
+  };
+};


Looks like this stuff didn't actually wind up changing, can you revert it to how it was? Removing the comment + using an implicit return.

(a helpful tip for debugging implicit returns: you can do stuff like this:

const mapStateToProps = state => ({ selectedProject: console.log(getSelectedProject(state)) || getSelectedProject(state), })

This way, you can log out the values of things without having to refactor it to have room for a statement at the top)

So glad I'm not the only one that does this.

joshwcomeau · 2018-09-04T16:34:15Z

src/components/ProjectConfigurationModal/ProjectConfigurationModal.js

+import React, { Component, Fragment } from 'react';
+import { connect } from 'react-redux';
+import styled from 'styled-components';
+// import moment from 'moment';


Nit: comment

joshwcomeau · 2018-09-04T20:58:13Z

src/sagas/save-project-settings.saga.test.js

+      // Should update state & close modal
+    });
+  });
+});


I think there's a bit more work to be done fleshing out these tests. I think it's important for new feature work that we have good test coverage :) we don't need to handle every edge-case, but we should go through the happy path and maybe the most common errors?

I can try to tackle these tests tomorrow if you haven't gotten around to them by then.

joshwcomeau · 2018-09-04T20:58:40Z

src/services/platform.services.js

+  remote.app.getAppPath(),
+  './node_modules/yarn/bin',
+  formatCommandForPlatform(PACKAGE_MANAGER)
+);


This file is now named platform.service.js (I noticed a similar thing on another branch, not sure how this happened :/)

Please integrate any changes into that other file

joshwcomeau · 2018-09-04T21:02:57Z

src/types.js

@@ -25,7 +25,7 @@ export type DependencyStatus =
  | 'updating'
  | 'deleting';
 export type DependencyLocation = 'dependencies' | 'devDependencies';
-export type QueueAction = 'install' | 'uninstall';
+export type QueueAction = 'install' | 'uninstall' | 'modify';


I believe modify can be removed now that we aren't adding to the queue

joshwcomeau · 2018-09-04T21:03:41Z

src/types.js

@@ -92,6 +92,7 @@ export type ProjectInternal = {
    color: string,
    icon: string,
    createdAt: number,
+    isImported: boolean,


This also appears unused, outside of the middleware (which has been replaced with a saga)

joshwcomeau · 2018-09-04T21:04:25Z

src/utils.js

@@ -227,7 +227,7 @@ export const getTimeOfDay = () => {
  }
 };

-export const hasPropChanged = (oldProps, newProps, key) => {
+export const hasPropChanged: boolean = (oldProps, newProps, key) => {


Nit: this annotation shouldn't be necessary, Flow is generally pretty good about inferring this. You can still add it, if you think it's helpful, but it isn't required.

AWolf81 · 2018-09-04T23:29:06Z

src/sagas/dependency.saga.test.js

-      expect(saga.next().value).toEqual(
-        takeEvery(START_NEXT_ACTION_IN_QUEUE, handleStartNextActionInQueue)
-      );
+      // expect(saga.next().value).toEqual( // Todo: Create queue.saga.test file


@superhawk610 I think we need to create a queue.saga.test and move these lines there. Also the other queue related Saga tests should be moved there.
Is it OK to address this after this PR?

Good catch!

AWolf81 · 2018-09-04T23:30:54Z

src/sagas/save-project-settings.saga.test.js

+
+      expect(saga.next().value).toEqual(
+        put(
+          saveProjectSettingsFinish(undefined, 'project', 'path/to/new-project') // why is project undefined?


@superhawk610 Do you have an idea why I have to pass undefined for the project here? Couldn't see what's wrong.

Ah, OK, I missed to pass the obejct to saga.next so there was no project passed. This is fixed. I'll push it in a minute.

AWolf81 · 2018-09-05T00:02:07Z

Phew, I hope I've added everything.

At the moment, there is one failing test. Because of the refactoring in projects.reducer to a new case - I'll fix this later today.

Yeah, sorry, that this is pretty large. But I think it's that large because I had to start with the Windows-support branch so I could start to work on it. I think furture features should be smaller.

For the ID change:
I think we could address this after merging so this PR isn't getting even larger. I like to have the id like the name but I think a UUID inside of Guppy key should be no problem.

joshwcomeau

Hey @AWolf81,

Wooo, it's looking great, thanks for making so many of the requested changes. The only request I found that wasn't addressed (either by changing the code, or in your comment above) was having SAVE_PROJECT_FINISH dismiss the modal instead of dispatching a hideModal(). This is not a big deal, though. I can tackle that afterwards, when I do some visual tweaks.

I do have a hesitation about approving this when there are still two things I think should be done:

Removing the logic to update the project ID when renaming
Adding tests for queue.saga.js

My rationale:

For the project-ID updating, my concern is that once we merge this in, other people will keep working on the code. If the code was contained to its own little section I'd be more OK with it, but it's in almost all of our main reducers (having to delete the entities and replace them with the new ID whenever the ID changes).
For the tests, I think we really wanna be pretty strict about making sure that all new code merged in has good test coverage, especially for critical business logic like managing the dependency queue.

I do hear what you're saying about this PR getting a bit long, so I have a proposal; what if you check out a new branch from this one, do those couple changes, and submit it for review (using this branch as the base instead of master). Then, we can approve that branch, merge it into this one, and then merge the whole thing into master.

Also, to clarify: I like the idea of using UUIDs for project IDs, but that's a bigger change than I think is necessary right now. For now, I think we can just leave the ID unchanged (so this should just be a matter of updating the ID-changing logic in save-project.saga, as well as removing the cases from the various reducers). I'd be happy to merge that in now, and create an issue to convert project IDs to uuids in the future.

Let me know how that sounds! I wanna try and be accommodating since you've worked so much on this, but I also think it's important to merge code we feel is 100% ready, especially as we get newer contributors who might build on whatever's in master.

joshwcomeau · 2018-09-05T19:25:11Z

src/components/SettingsButton/SettingsButton.js

+  render() {
+    const { hovered } = this.state;
+    return (
+      // Omitting most of the structure that isn't relevant


What does this comment mean, what structure is omitted?

mmhhh, not sure. I think that was a comment that was in your proposal. 😄
Maybe this is outdated. I'll remove it.

joshwcomeau · 2018-09-05T19:26:12Z

src/reducers/modal.reducer.js

@@ -19,13 +21,18 @@ type State = 'new-project-wizard' | null;
 const initialState = null;

 export default (state: State = initialState, action: Action) => {
+  // console.log('action', action);


stray comment

joshwcomeau · 2018-09-05T19:28:39Z

src/reducers/projects.reducer.js

@@ -165,6 +185,7 @@ export const getInternalProjectById = (state: GlobalState, id: string) =>
  getById(state)[id];

 export const getProjectsArray = (state: GlobalState) => {
+  // console.log('get projects', state.projects);


stray console.logs, here and on line 119

joshwcomeau · 2018-09-05T19:31:15Z

src/reducers/queue.reducer.js

@@ -26,6 +26,10 @@ type State = {

 const initialState = {};

+// const addToQueue = (queueType: string, draftState: any) => {
+
+// }


stray comment

joshwcomeau · 2018-09-05T19:32:16Z

src/sagas/dependency.saga.test.js

-      expect(saga.next().value).toEqual(
-        takeEvery(START_NEXT_ACTION_IN_QUEUE, handleStartNextActionInQueue)
-      );
+      // expect(saga.next().value).toEqual( // Todo: Create queue.saga.test file


Good catch!

* added uuid & removed id change * added root test * isolate queue handling to queue saga * removed commented code. * move all queue testing to queue saga test

to use byId

joshwcomeau

Yay excited to see this land! =D

As discussed, I'm gonna open a subsequent PR to tweak a couple things, but it shouldn't affect things too much, so I'm happy to merge this in now.

bennygenel and others added 25 commits July 16, 2018 13:36

Initial changes

edfc88d

removed console.log and added prettier-ignore

df57950

fixes typo and missing isWin control, adds platform.services

c63ecc2

temporary solution for the eject error

e8ed3f3

added missing declarations on platform.services

eca70f0

Initial changes

34c0f50

removed console.log and added prettier-ignore

ecd609c

fixes typo and missing isWin control, adds platform.services

93e118c

temporary solution for the eject error

f5f6dfb

added missing declarations on platform.services

07a391d

redo task start commands

8595954

Merge branch 'windows-support' of https://github.com/bennygenel/guppy …

bfc7284

…into windows-support modified: scripts/start.js modified: src/middlewares/task.middleware.js

changed to yarn using package-manager.service

765fc31

used platform.service & re-structured PACKAGE_MANAGERS constant

4c48f6e

using PACKAGE_MANAGER_CMD from platform.service

3db32e5

used spawnProcess from logger branch.

05821bf

added app config

88ba688

removed package-manager.service

e7eb1fa

added yarn locally to Guppy.

d5db626

added settings gear button & modal

02d8496

WIP: Modified settings button.

53119b0

WIP: Changed to redux modal. Next add business logic for save settings.

5a8cb27

WIP: Added renaming. Fails at CHANGE_PROJECT_NAME_FINISHED

577a9f5

added scale onHover

d304465

added confirm dialog for imported projects & modified renaming logic.

52ef635

AWolf81 added 3 commits August 16, 2018 23:28

Merge branch 'master' into project-configuration

556b989

added cursor pointer.

cdc67b4

improved state updating after save.

ecf7c47

melanieseltzer mentioned this pull request Aug 17, 2018

Add delete project from project list feature #90

Closed

AWolf81 added 6 commits August 31, 2018 01:19

Merge branch 'master' into project-configuration

7516224

WIP: Updating state needs to be checked.

eb43f7f

Merge branch 'master' into project-configuration

04ad7b0

WIP: Added confirmation for imported projects.

6c197be

Merge branch 'master' into project-configuration

bd6721f

WIP: Adding queue to project settings saving.

32c9fd4

superhawk610 mentioned this pull request Sep 3, 2018

Unify pattern for queue starter actions #212

Merged

AWolf81 added 3 commits September 3, 2018 23:05

WIP: Adding modify queue.

19c2c26

added project save disable if dep. queue not empty

cdb5205

WIP: Add save-project-settings.saga.test

6249095

joshwcomeau suggested changes Sep 4, 2018

View reviewed changes

AWolf81 added 3 commits September 4, 2018 23:59

remove unused import & comment

d8fcc3c

smaller tweaks.

ffaef50

addressed review changes

17365de

AWolf81 commented Sep 4, 2018

View reviewed changes

Merge branch 'master' into project-configuration

1c2c986

Fixed tests & removed comments

0a5ffab

joshwcomeau reviewed Sep 5, 2018

View reviewed changes

AWolf81 and others added 3 commits September 7, 2018 22:46

Final tweaks to PR joshwcomeau#120 (#1)

72b7660

* added uuid & removed id change * added root test * isolate queue handling to queue saga * removed commented code. * move all queue testing to queue saga test

removed comments

023c15a

WIP: prepared save-project-settings for new projectPath handling

6ceb180

AWolf81 mentioned this pull request Sep 8, 2018

Allow custom project name #229

Closed

AWolf81 added 3 commits September 9, 2018 14:03

Merge branch 'master' & fixed saving in paths.reducer

897d5ca

to use byId

Merge branch 'project-configuration'

6955e79

removed comment

13d0da8

AWolf81 mentioned this pull request Sep 9, 2018

Refactoring - Create ProjectIconSelection Component #232

Closed

joshwcomeau approved these changes Sep 9, 2018

View reviewed changes

joshwcomeau merged commit 4593736 into joshwcomeau:master Sep 9, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Project configuration #120

Project configuration #120

AWolf81 commented Aug 6, 2018 •

edited

Loading

superhawk610 commented Aug 13, 2018

joshwcomeau left a comment

joshwcomeau Aug 18, 2018

joshwcomeau Sep 4, 2018

joshwcomeau Sep 4, 2018

AWolf81 Sep 4, 2018

joshwcomeau Sep 4, 2018

AWolf81 Sep 4, 2018

joshwcomeau Sep 4, 2018

superhawk610 Sep 4, 2018

joshwcomeau Sep 4, 2018

joshwcomeau Sep 4, 2018

superhawk610 Sep 4, 2018

joshwcomeau Sep 4, 2018

joshwcomeau Sep 4, 2018

joshwcomeau Sep 4, 2018

joshwcomeau Sep 4, 2018

AWolf81 Sep 4, 2018

joshwcomeau Sep 5, 2018

AWolf81 Sep 4, 2018

AWolf81 Sep 5, 2018

AWolf81 commented Sep 5, 2018

joshwcomeau left a comment •

edited

Loading

joshwcomeau Sep 5, 2018

AWolf81 Sep 5, 2018

joshwcomeau Sep 5, 2018

joshwcomeau Sep 5, 2018

joshwcomeau Sep 5, 2018

joshwcomeau Sep 5, 2018

joshwcomeau left a comment

Project configuration #120

Project configuration #120

Conversation

AWolf81 commented Aug 6, 2018 • edited Loading

superhawk610 commented Aug 13, 2018

joshwcomeau left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AWolf81 commented Sep 5, 2018

joshwcomeau left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshwcomeau left a comment

Choose a reason for hiding this comment

AWolf81 commented Aug 6, 2018 •

edited

Loading

joshwcomeau left a comment •

edited

Loading