Project configuration

[joshwcomeau]

Project configuration

[joshwcomeau]

Thanks so much for all the work you've been putting in on this. Sorry it's taken me a few days to reply D: I should become more responsive on this project in a week and a half, after React Rally.

should we add the size of the randomSubSet to 10 or is there no need to pass it with a prop? Or we could also pass it with randomSubSet={?number} instead of a boolean.

I'm not super picky :) I think a number works! I'm happy either way

Should I create this component at this issue?

Yeah, I'd group it in. Not substantial enough for its own issue. If you wanted to create a specific PR for it, that could be a good idea (lots of small PRs vs. one large one can be easier for reviewers), but I also recognize the burden of managing multiple PRs, so feel free to lump it in with the larger change.

Oh, sorry, I missed that component. What do you think should I open an issue for a feature request for Storybook? That would help to see all available components in action with some docs for the usage.

Yeah, so I use Storybook often in side projects. I'm not sure why I didn't in this project, I guess I was just in a hurry :/

The only hesitation with Storybook is that adding stories is non-trivial and takes some time. I don't know who would do that work / where the time would come from, as it's not something I can commit to myself.

I know there are other solutions like Styleguidist that I think involve less upfront work?

Either way, I think we should create an issue for this; even if we don't create a full set of stories, it'd be good to add stories for the primitives like TextInput, I might be able to knock a few of them out so that they're more discoverable.

[AWolf81]

We can close this as it is merged to master. 🎉

[joshwcomeau]

Note: Deleting a project is tracked in #90, so this issue can just track changing the project's name/icon

[AWolf81]

I'd like to work on this issue. I like the idea of the modal and the gear icon.

I'm working on branch project-configuration.

I have some questions to the feature:

• Renaming
  • What's affected by the rename?
    • Project folder or just the project name in package.json? Or both?
    • package.json inside key Guppy - change id & name?
    • package.json name key changed to same value as Guppy.id?
    • is renaming locked if any process of project is running (e.g. devserver, install or remove dependency)? Could be a problem with access problems.
• Where to add the gear settings button? Is it in ProjectPage component after the heading pixel shifter OK?
• Is a new reducer needed for the renaming and icon change?
• Should we create a new service for project related processes? E.g. rename folder or modify package.json to the new name. I would call it project-settings.service
• Is project-name.service required for renaming? Where is it used?

Current progress (todolist moved to PR)

• Add Gear button to project pane to open modal
• 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 (optional - maybe add 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
  • Reselect the current project with the new projectId
• Renaming for imported projects with confirmation
• Improve styling of project name text input & buttons
• Check if package.json requires locking - wait for #103
• Create temporary middleware for renaming & modifying project folder & package.json - later change to Saga
• Check initial loading? Maybe icons need a loading indication.

Issues:

• Currently selected project icon does not appear in iconsubset iconSubset = sampleMany(iconSrcs, 10); - How to add that icon so it can be selected in picker? For now loaded all icons.

Draft gear icon (not animated)

Draft settings modal (not latest version)

Screen recording (gear & modal)

[joshwcomeau]

Hi @AWolf81,

Yay, thanks for taking this on!

I have some questions to the feature:

Renaming
What's affected by the rename?
Project folder or just the project name in package.json? Or both?
package.json inside key Guppy - change id & name?
package.json name key changed to same value as Guppy.id?

Hm, yeah. I'm curious what others think. My instinct is that for projects created through Guppy, it should 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. Also rename the parent folder.

For projects imported, though, it's murkier to me. In that case, I wonder if we should just change the stuff in package.json's guppy key.

Although, I don't love the idea of a split code path like that. I'd much rather behaviour was consistent.

Maybe we can do all the same stuff for imported projects, but we can prompt with a warning before making the change. Or, maybe we can disable renaming for imported projects?

I think maybe for now, we can just disable renaming for imported projects. If we get a lot of pushback on that from users, we can figure out what the best way to handle it would be?

is renaming locked if any process of project is running (e.g. devserver, install or remove dependency)? Could be a problem with access problems.

Yeah, good point. In #103, we're moving away from having things "locked", and instead having a queue for actions that affect package.json. Renaming doesn't really make sense as part of the queue, though. Gosh, this really shows the brittleness of NPM's dependency systems, right? The fact that all of the settings live in the same manifest as the dependencies, and those settings can't be tweaked while dependencies are installed :/

Once #103 lands, I think there will be a way to derive whether or not the package.json is locked, and we can selectively disable the UI when that's true. But for now, I think we can punt on this issue. Definitely needs to be fixed before this feature launches, but doesn't need to be a part of this PR.

Where to add the gear settings button? Is it in ProjectPage component after the heading pixel shifter OK?

In terms of on-screen location, that looks great!

In terms of component architecture, I think I'd do something like this:

<MainContentWrapper>
  <FlexRow>
    <PixelShifter>
      <Heading>{project.name}</Heading>
    </PixelShifter>
    <SettingsButton />
  </FlexRow>
</MainContentWrapper>

FlexRow would be a styled-component that just sets display: flex; justify-content: space-between. Maybe align-items: center for vertical alignment as well.

I haven't been consistent with whether or not to use Redux for modals, but I think we should use it here. That way, SettingsButton can be redux-connected to pull in the actions to open the modal, and the actual <SettingsModal> can live in App.js by CreateNewProjectWizard. I like the idea of SetingsButton being a batteries-included component we can drop anywhere.

Is a new reducer needed for the renaming and icon change?

I don't think so; the only state is whether or not the modal is open, and I think that can be tracked in modal.reducer.js. We can use React state for the "transient" values (like what's in the input), and dispatch an action to update projects.reducer with the new value (and also update the underlying package.json in a saga).

Should we create a new service for project related processes? E.g. rename folder or modify package.json to the new name. I would call it project-settings.service

I think that makes sense, but it might be a small enough amount of filesystem interaction that it's fine to just do it in the middleware/saga. I have some thoughts on this below.

Is project-name.service required for renaming? Where is it used?

project-name.service is just for generating random names. It's a misleading filename, should be generate-random-name.service or something.

---

Aside from your questions, I have some additional thoughts:

1. In #117 and #103 (among others), we're converting all middlewares to use Redux Saga. This was done because Sagas deal much better with complex async flows, they're easier to test, and they make for much more readable code. So there's some coordination with that that'll be required. Maybe create a temporary middleware for now with the understanding that once those PRs land, we can update it to be a saga?

2. For the gear icon, can we use a softer gray, maybe gray[600] or gray[700]?

3. I'd love to inject some whimsy in that icon. Maybe on hover it can spin rapidly for a second or so? We could use React Motion to make this feel really organic and nice:

class SettingsButton extends Component {
  state = {
    rotations: 0,
  }

  handleMouseEnter = () => {
    // We can try a bunch of numbers here.
    // 0.5? 1? 2? 5?
    const numOfRotationsOnHover = 1;
    this.setState(state => ({
      rotations: state.rotations + numOfRotationsOnHover, 
    }))
  }

  handleMouseLeave = () => {
    // I wonder if we should "unwind" it on mouseout?
    // I'm not sure... maybe try it with/without this?
    this.setState({ rotations: 0 });
  }

  render() {
    return (
      // Omitting most of the structure that isn't relevant
      <Motion style={{ rotations: spring(this.state.rotations) }}>
        {({ rotations }) => (
          <IconBase 
            icon={gear} 
            style={{transform: `rotate(${rotations * 360})deg`}}
            onMouseEnter={this.handleMouseEnter}
            onMouseLeave={this.handleMouseLeave}
          />
        )}
    );
  }
}

It should also have a different color on hover, maybe blue or purple?

4. For the form itself, I think there should be a large, centered green button at the bottom that says "Save" (and maybe another that says "cancel"?), rather than having it be next to the project name input. Also, I think the project name input should be "Project name" instead of "Change project name".

5. In the create-project-wizard, we grab a random subset of the available icons. For this, maybe we should show all of them? Right now we don't have too many, so I think it's feasible. We could sort them alphabetically, which would group similar ones together.

---

Phew, sorry for writing a short novel here. Let me know if anything is unclear!

[AWolf81]

@joshwcomeau Thanks, for the invitation to collaborate at the project.

And thanks for the detailed explanation. I'll go through the details and ask if anything is unclear.
It's also OK that it is a short novel, my question was also pretty long.

At first glance I thought that's easy to add but it's a pretty complex topic - but should be doable.

I'll add your details to my todo list later.

Renaming:
Why not handle imported and created projects identically? The guppy key will be added during import to the package.json, right?
Then Guppy is in control and can change the directory of the project and everything in the guppy keys - everything like at a guppy created project.
The imported project is copied to the guppy project folder - so the user will not lose anything as the imported location still exists after importing.
But if we're not sure how to handle we can disable renaming for imported project. The problem is just not clear to me.

Modals
OK, I've read in the code that the modal handling is not consistent and started to do it with a component based modal similar to TaskDetailsModal. But that's OK, I'll change this to Redux based modal as this is probably a better fit. So it's re-usable for many settings that we may need.

to 1. OK, I'll create a temporary middlware for this, that needs to be modified to Sagas later.

to 2. Changing the gear to a softer gray is no problem - can you please give some details how the number thing at the color constants is working? Higher number is a darker color, right? How is this scaled 50 to 900 is the key of the color object?

to 3. I love the idea with the spinning gear icon - I'll check how it will look.

to 5. Ah, OK, I'll try to display all icons and check if this will work.

[joshwcomeau]

The imported project is copied to the guppy project folder - so the user will not lose anything as the imported location still exists after importing.

Huh. So this actually isn't true - we don't copy the imported project, we modify the source project. Copying the project hadn't occurred to me at all, but I kinda wish it had, since it would have solved a lot of project-path issues, haha.

Although it might be really confusing to users who don't realize that Guppy creates its own copy, and who then can't tell why the changes they make to the original location aren't reflected in Guppy.

And so yeah, my hesitation to renaming copied projects is that we'd have to modify the original package.json's name (and maybe even its location in the filesystem if we rename the directory). I'm OK with modifying the package.json to add the guppy key since that's not changing anything pre-existing, it's just adding a new thing... this feels more invasive though.

OK, I've read in the code that the modal handling is not consistent and started to do it with a component based modal similar to TaskDetailsModal. But that's OK, I'll change this to Redux based modal as this is probably a better fit. So it's re-usable for many settings that we may need.

Yeah, sorry about that. I think redux is a better fit here because otherwise we'd either need to embed the modal within <SettingsButton>, which feels wrong, or have the modal live in ProjectPage and pass a callback to the button to open/close it, which feels better but also means that the <SettingsButton> is less portable.

We should update TaskDetailsModal to use Redux as well. I'll create an issue for it.

Changing the gear to a softer gray is no problem - can you please give some details how the number thing at the color constants is working? Higher number is a darker color, right? How is this scaled 50 to 900 is the key of the color object?

Higher numbers are darker, indeed. You can see the definition in constants.js. I'm not sure that I understand the latter part of your question. The keys are technically strings (so it should be gray['600'], but we can take advantage of Javascript's coercion to convert the number to a string). In retrospect it's probably pretty confusing.

[AWolf81]

Ah OK, sorry, I thought I have seen this but in my current working directory it is not copying the project - maybe that was a mistake in an old branch but I'm not sure.
And you're right a copy can be confusing.

So I think we have three options to deal with imported projects:

1. Show a confirmation so the user can decide if the project folder should be renamed as well.
2. Rename folder with-out notification
3. Disable folder rename for imported projects

I think I would prefer option 1 but option 3 is also OK.

The question to the color constants - I wanted to know why you picked the numbers like this? I would have used something like 100 for 100% brightness and 10 for 10% brightness. Or if there are only two shades something like dark or light as key - but 50 to 600 is OK but a bit confusing.

[joshwcomeau]

Hm, yeah, so Option 1 is indeed the best UX IMO, but I'm wondering if the added code complexity is worth it. I suppose it wouldn't be too much extra complexity, since presumably the only place that it matters is in the new middleware/saga (or associated service).

If you're up for Option 1, go for it - you can use the native prompts like we use for the pre-eject warning.

The question to the color constants - I wanted to know why you picked the numbers like this? I would have used something like 100 for 100% brightness and 10 for 10% brightness. Or if there are only two shades something like dark or light as key - but 50 to 600 is OK but a bit confusing.

Ah, right. Yeah I don't have a great rationale; it's mimicking font-weights, where they're 100-900 where larger numbers are heavier. I think I inherited this style from someone else years ago. I added 50 because I needed an off-white.

Given that there are 10 greys, I think a number makes more sense than text, since there's too many. And for other numbers like red, it's not simply a matter of tweaking the brightness (so it might be misleading if you thought that red[50] was just red[100] at 50% brightness/saturation/whatever)

[AWolf81]

@joshwcomeau OK, thanks for explaining the color numbers. I think this is good to know that is similar to font weight and you're right percentages are probably no good fit.

Update to my current progress:

Gear animation

Please have a look at the screen recording after my todo list (or check in my branch). Added rotation + color change from gray to purple. Maybe opacity fading & color switching would give a similar effect. The color change is not that visible.
In your component code in your comment there is a typo at tranform rotate - the brace should be after deg. Took my some time to figure it out why it wasn't rotating. Beside this it's working great.
What do you think should we slightly increase the size of the gear on hover & shrink on leave? I think this could be a nice effect.
Spinning is just a small amount as it was too fast for larger values.

Modal

Can you please have a look at my modal code? I've change it a bit so it's working with Redux. I just don't like the dynamically loading of the modal component - it's working but maybe there is a better way to solve this.

I've added the Modal component to app.js. I'm loading the ModalContent based on the state.modal value - here ProjectConfigurationModal.

The component ProjectConfigurationModal is loaded into MODALS object with require. If state.modal is defined the modal will be visible and display the content.

[AWolf81]

@joshwcomeau I have a problem in project.middleware in my branch (temporary middleware responsible for renaming & icon change)
Do you have an idea what I'm doing wrong?

The project folder is renamed (no check for imported project yet) and the package.json is correctly modified. Then I'd like to refreshProjects inside CHANGE_PROJECT_NAME_FINISHED. But I'm getting an error that name is undefined in ProjectConfigurationModal if I'm dispatching refreshProjects.

I'll check this tomorrow. Seems like the modal is re-rendering. Maybe I have to close the modal first and then refresh the projects and select the updated projectId.

[joshwcomeau]

Hey @AWolf81,

Spin animation looks great! I'm into the idea of a slight size increase with scale(). Sorry for the typo in the sample code!

Yeah, so that error makes sense to me; I ran into a similar one with ejecting while the TaskDetailsModal was open. To fix it, I think we'll need to update the selectedProjectId in projects.reducer.js when the project name is changed; that way, there is never a moment where the selected project ID refers to a project that doesn't exist.

So I'd add a case to projects.reducer that listens for CHANGE_PROJECT_NAME_FINISHED and sets the selectedProjectId to the new ID.

I looked through your branch's code and overall I think things are on the right track. Some notes:

1. I don't love the MODALS object, and the dynamic switching of the various imports. The way it is currently dealt with, App always renders <CreateNewProjectWizard />, and that component internally decides whether to render or not.

I think I prefer this approach, with App not having to decide which modal to render. It's too easy for the top-level App component to have too many concerns to deal with, and so I'd like to keep it as simple as possible. Is there a reason it couldn't work for ProjectConfigurationModal as well?

Another issue with your new approach is that CreateNewProjectWizard has this special foldable-modal that it controls internally, so it shouldn't be nested inside like this.

I think if we do want to move the rendering outside of the specific components, we could do something like this:

type Props = {
  currentModal: 'new-project' | 'project-settings',
}

class CurrentModal extends Component<Props> {
  render() {
    return (
      <Fragment>
        {currentModal === 'new-project' && <CreateNewProjectWizard />}
        {currentModal === 'project-settings' && <ProjectSettingsModal />}
      </Fragment>
    )
  }
}

...and then in App, we would just render <CurrentModal />.

But yeah, again, I'm not sure we need to do this; I think I'd rather ProjectConfigurationModal just grab whether it's visible from Redux, and pass that data onto Modal:

class ProjectConfigurationModal extends Component {
  render() {
    return (
      <Modal isVisible={this.props.isVisible}>
        // stuff here
      </Modal>
    )
  }
}

const mapStateToProps = state => ({
  isVisible: state.modal === 'project-configuration',
})

(I didn't run any of this code, mostly this is just an idea, not something to be copy-pasted)

2. I noticed you started using Flow with Redux actions - I think this is a good refactor, but I think I'd rather it be done separately, so that all actions can be converted and it doesn't create so much noise in this PR. Definitely like the approach of having types for the action-creators though!

3. It looks like there's still a specific action for renaming the project, whereas I think it'd be better if there was a single "Save" button for the whole form. In this case I think the actions would change to just be SAVE_PROJECT_SETTINGS_START and SAVE_PROJECT_SETTINGS_FINISH (and maybe an error one as well).

Some other smaller notes for React bits:

• there's a TextInput component you could use instead of <input type="text">
• I think we should have a new component for selecting icons that is used in both modals, I'm imaging an API like this:

<ProjectIconSelection
  selectedIcon={?string}
  showRandomSubset={boolean}
  onSelectIcon={function}
/>

(used Flow types instead of actual values, to give an idea what I'm picturing).

The reason for this is that there's actually a fair bit of logic with icons, like having them go gray when none is selected, to the business with importAll.sync. It would be great to DRY that stuff up

---

Overall I think this is on the right track, though :) lemme know if my proposed fix for the modal issue doesn't work!

[joshwcomeau]

Oh, also: could you open a PR for this, even if it's obviously WIP? It was surprisingly hard to look at the changes (for some reason everything I tried to compare it to had a bunch of extraneous commits, both on your repo and mine, so I had to go commit-by-commit. A PR would help me see all the changes together)

[AWolf81]

@joshwcomeau OK, I've opened a PR.
Yes, you're right with the Modal and I think we only need to add the ProjectConfigurationModal to app.js after the CreateNewProjectWizard - I thought to abstract the modals into a component would help to reuse the modals but I think for two modals it's OK to add them to app.js and handle visibility with state.modal.

To 2. I created a separate issue to have a reminder for this and I'll remove the types from the actions for now.

To 3. That makes sense and I'll modify this in my code. To have just one save action for project name & icon.

ProjectIconSelection component
That's right - this will help to keep things DRY. I like the API, should we add the size of the randomSubSet to 10 or is there no need to pass it with a prop? Or we could also pass it with randomSubSet={?number} instead of a boolean. If randomSubSet is not passed it could render all icons.
Should I create this component at this issue? I think this is a small task and should be no big deal to add.

TextInput component
Oh, sorry, I missed that component. What do you think should I open an issue for a feature request for Storybook? That would help to see all available components in action with some docs for the usage.