add the support of selecting projectPath when creating new project #70
Conversation
|
IMO path inputs, especially ones without autocomplete, are bad UX, and I feel like we shouldn’t be directly exposing new coders to the concept of “paths” before they even get started coding. How about having a button or a link to open a dialog that would let the user select a path using the native system UI? |
|
From #35 (comment)
Although the topic is different on that issue, I agree with the thought and I think it applies to this too. For a beginner user these and similar questions are always there. Giving user lots of thing to choose from adds to these questions. "Are these choices revertible?", "Can I change this folder?" etc. Having some sort of an "Advanced" toggle or a modal is a better way to go IMO. With default settings everything should work without asking for detailed information. |
|
@j-f1 @bennygenel Actually, this text-input is readonly and only display selected path. it will open a dialog to go IMO with the native system UI when clicking. |
|
I think this is a needed addition. Thank you for the Pull Request. The choice, or at least awareness, of the project location is actually important to a first time user, so that they can later find their project in Finder/Explorer. The way it is handled now, with a folder in the top level home folder, is not even accessible via the default Finder sidebar I believe. If we wanted to go down that route, we need to add it ourselves, like Dropbox does, but I don't think guppy is that kind of app. I would imagine something like this (in structure, not design details) would serve the purpose. In functionality, this is very close to what you already have I think. How would you feel about reworking the ProjectPath component to something like that @kermit-xuan ? |
|
Yes, you are right. How about setting default projectPath by
|
|
I think you can drop the TextField altogether and simply display the path as text, since it is read only and can't be submitted with enter anyway. There's a TextButton component you can use to open the file picker. It is also used for "Import it instead" in the screenshot there. However @joshwcomeau might want to chime in on the design choices here. |
|
Yeah, so my sentiment is similar to @bennygenel's; I want to make the process as simple as possible, making reasonable assumptions instead of adding yet another prompt for the user. I think having the user select a path means that they have more unanswered questions, not less. Incidentally, I think a very important thing that we currently don't do is show the user where their project lives. This will be addressed in #46. So yeah, I'm a bit resistant to this change in general. @karlsander brings up a really good point, that the current path isn't easily accessible, so I think we should move it to I might be wrong about how important this is to the beginner experience... but I'd like to wait and see if that truly is a problem before addressing it. I suspect that beginners won't care about where it's stored as long as they can find it, and advanced users will have no problem circumventing the lack of a first-class input for it. Sorry if you invested a lot of time in this :/ hopefully in the future we can start a discussion in an issue before investing significant time in issues, to make sure there's a consensus around its desirability? That's where my head's at now, but I encourage y'all to chime in and let me know what you think about the alternative idea of better visibility + import instructions |
Going to agree with @karlsander here. Just gave the app a go (great job btw!) and tried to create a new project as a test. A minute in I realized it had started installing (there was another issue here with the inability to cancel the installation but that's for another thread) and I was never asked where this was saving. Had to fish around Finder to locate it. Would be good to get the option. |
|
I think it is very necessary to choose the project path. As far as I am concerned, my private projects and work projects are two different workspaces, and I don't want to change the original project path when I import the project.
|
|
@kermit-xuan I suppose my intention misunderstood. I have never said let's not put it at all. I think it should be more informative that's all. That's why I suggested "Advanced" section. I remember my first days of started coding and when I see a section called Advanced I simply thought "it can be left as is until I know more...". Maybe my suggestion was for future iterations. I just added my thoughts. I also think user should be able to select the path, I even think that there should be a "Move Project" option too. With the current setup if you move project manually you end up with an error. Even if we pass that error, you need to import the project again. But I think this is something to discuss when the "Delete Project" option going to be added. For now I just suggest to be a little bit more clear about the selections. Like having a nice description with the detailed information link on left pane when you select the Project Type, there can be a small description about the path too. Just to show my intention |
Yeah, fair point. Alright, I'm coming around on this. I think as long as it's subtle and something that doesn't require input if the user doesn't want to specify, I'm on board. I'm realizing that the existing new project flow isn't as flexible as I'd like. I put so much care into the animations and sequencing that it's actually made it harder to change :/ if we add that field in, we run out of space for the icons. I suppose we can make the modal a bit taller; current modal is ~520px tall, so if we increase it to 560px or whatever it'll still fit without needing inter-modal scrolling. Gonna give the code a review now |
There was a problem hiding this comment.
Thanks for your work on this issue.
I'm coming around to the importance of having this as an option, but I think we should be able to reuse the custom paths mechanism for imported projects.
I described the situation in an inline comment, but feel free to follow up if you have any questions!
src/base.css
Outdated
| @@ -4,6 +4,7 @@ button, | |||
| select, | |||
| option { | |||
| font-family: 'Futura PT'; | |||
| outline: none; | |||
src/utils.js
Outdated
| const list = getProjectList(); | ||
| list[id] = path; | ||
| localStorage.setItem('project-list', JSON.stringify(list)); | ||
| }; |
There was a problem hiding this comment.
Ah, so localStorage doesn't actually persist between Guppy sessions in the built version. We have to use Electron storage instead.
One maybe-surprising bit of how Guppy projects are organized is that paths to the projects live in a separate reducer, paths.reducer.js. The reason for this is that paths are not specific to a project necessarily (the same project might live in different places on different machines).
We already store custom paths with the import-project workflow. I think most of that should be reusable for this.
Essentially, CREATE_NEW_PROJECT_FINISH should include the projectPath that was stored in CreateNewProjectWizard state, and paths.reducer.js should listen for this action and add a key for it.
Everything else should work automatically, you shouldn't need either of these utils, or indeed most of the code you have outside of the wizard :)
|
@joshwcomeau Thank you very much for review. I'm not good at React, and hope to get promotion by practice with React. I have optimized code with |
|
Hi @kermit-xuan, Sorry so much for the delay on this :( I was busy with preparing for React Rally, and this slipped through the cracks. We have a Gitter channel, in the future if you ever have a PR that doesn't get reviewed after 24 hours, just post something in the channel and I or one of the other contributors will give it a review and merge it if it's ready :) I hope the delay didn't discourage you from working on this project! We'd be happy to help guide you to learn React. I'm gonna re-open your PR, and submit a review for it. It's almost ready, I just have a couple more tweaks I'd like to see happen. If you're not interested in working on this anymore, that's OK - I do think this is important functionality though, so I'd like to take over and implement the changes I request myself. Just let me know how you'd like to proceed :) |
src/reducers/paths.reducer.js
Outdated
| const { path } = action; | ||
| return { | ||
| ...state, | ||
| defaultProjectPath: path, |
There was a problem hiding this comment.
The current structure for this reducer's state is a map-like object:
{
'first-react-app': 'path/to/project/',
'second-react-app': 'path/to/project/',
}Because of that, I think it's a bit weird if there's also a defaultProjectPath living as a sibling to these projects. For one thing, if someone oddly chooses to name their project defaultProjectPath, it would break.
I think we'd need to restructure the reducer so that it has this kind of shape:
{
defaultHome: '~/guppy-projects',
byId: {
'first-react-app': 'path/to/project/',
'second-react-app': 'path/to/project/',
},
}
This way, it's clear that there's a distinction between project paths and the default value. I like the idea of changing the names so that the "parentPath" just becomes the "home" for the projects.
It does mean that some refactoring would be necessary: getPathForProjectId selector would need to be updated to use the new structure.
src/actions/index.js
Outdated
| export const setDefaultProjectPath = (path: string) => ({ | ||
| type: SET_DEFAULT_PROJECT_PATH, | ||
| path, | ||
| }); |
There was a problem hiding this comment.
Could you please rename this action to changeDefaultProjectHome?
- I think
changeis better thansetbecause it's clear that the user is changing something, and it's not just setting a property on a model. It's user-directed. homePathinstead ofpathbecause it's setting the parent "home" directory, not the project directory itself.
| return ( | ||
| <MainText> | ||
| created in | ||
| <Tooltip title={defaultProjectPath} position="bottom"> |
There was a problem hiding this comment.
Nice touch, using the tooltip!
src/reducers/paths.reducer.js
Outdated
| // Noticing some weird quirks when I try to use a dev project on the compiled | ||
| // "production" app, so separating their home paths should help. | ||
| process.env.NODE_ENV === 'development' | ||
| const { paths = {} } = getInitialState(); |
There was a problem hiding this comment.
If this needs access to the state, it should become a selector instead of a helper.
I have an idea for how to clean this up a bit. Let's use the getDefaultParentPath helper logic for the initial value for the state:
const initialState = {
defaultHome: process.env.NODE_ENV === 'development'
? `${os.homedir()}/guppy-projects-dev`
: `${os.homedir()}/guppy-projects`,
byId: {},
}Then, the selector can just return that value:
export const getDefaultProjectHomePath = (state: State) => {
return state.paths.defaultHome;
}You can use this in mapStateToProps by passing it the state.
There was a problem hiding this comment.
Need to be careful about the migration tho, it will be undefined for existing users
|
@joshwcomeau, I will resolve them, thank for your review. |
There was a problem hiding this comment.
Thanks for your work on this, @kermit-xuan!
I realized that, unfortunately, things are more complicated than I had thought. There are a couple reasons for this:
-
As I discuss in the inline comments, we need to make some more structural changes now that the home path info lives in Redux.
-
When I first cloned your repo, it crashed when I tried to open it. This is because of how all of the data is currently being stored in
electron-store. It provides the saved Redux data to the new Redux reducer, and it explodes.
I'm going to spend some time this weekend working on that second bullet point, it should hopefully clean a lot of things up! But in the meantime, I have some feedback for how you can address that first point.
Really appreciate the work you're putting into this! Sorry it ballooned into a bigger task
| message: 'Select the directory of Project', | ||
| properties: ['openDirectory'], | ||
| }, | ||
| paths => { |
There was a problem hiding this comment.
Nit: because only a single path can be selected, it'd be good to add Flow annotation:
(paths: ?[string]) => {
This annotation says it might be null, or an array with a single string in it.
src/reducers/paths.reducer.js
Outdated
| @@ -43,20 +82,21 @@ export default (state: State = initialState, action: Action) => { | |||
| // | |||
| // | |||
| // Helpers | |||
| const homedir = isWin ? windowsHomeDir : os.homedir(); | |||
| // Noticing some weird quirks when I try to use a dev project on the compiled | |||
| // "production" app, so separating their home paths should help. | |||
There was a problem hiding this comment.
Can you move this comment up above, to line 35?
| readdirSync(defaultParentPath).forEach(f => { | ||
| const projectPath = path.join(defaultParentPath, f); | ||
| readdirSync(getDefaultProjectHomePath()).forEach(f => { | ||
| const projectPath = path.join(getDefaultProjectHomePath(), f); | ||
| const isDirectory = statSync(projectPath).isDirectory(); | ||
|
|
||
| if (isDirectory && !projectPaths.includes(projectPath)) { |
There was a problem hiding this comment.
Right, so I actually don't want to do this stuff anymore.
I'll be working today on a fix that will remove this check. It doesn't make as much sense in a world where the home path is changeable, plus it makes some things a lot more complicated. So you can remove this whole block (lines 61-79)
|
Hi @kermit-xuan, Just a heads-up that the Redux migration stuff has landed in master :) I'd be happy to help write the migration for this change, so you can just focus on the "new user" experience. Also totally OK if you no longer feel motivated to work on this feature :) it's understandable, since things wound up being more complicated, and now there are some conflicts to resolve, etc. Just let me know, maybe I or another contributor can carry this over the finish line! |
|
I have solved them and are currently testing. |
There was a problem hiding this comment.
@kermit-xuan this is awesome. Thanks so much for all your work on this feature, it looks great! 🌟
|
@kermit-xuan I sent an invitation to become a collaborator on Guppy :) I really appreciate the work you've done. You've had a great attitude, not getting too discouraged when we realized there was more complexity than we thought, and going above-and-beyond to write migrations (+ tests!). Collaborators have access to the repo (so feel free to create new branches on this repo directly instead of your own fork), and can merge other people's code reviews, manage issues, etc. Of course this is all opt-in, no additional work is expected of you :) For more info, see the contributing docs Thanks again! |
The Support Of Selecting ProjectPath
Add projectPath textInput to select projectPath in MainPane, This is convenient to select project directory for different project.