App settings

[AWolf81]

Related Issue:

288

Summary:

• Added Application menu Linux/Windows at File/Preferences... & MacOs at Guppy/Preferences.... (MacOs & Linux not tested yet.)
• Added Settings Modal for defaultProjectPath, defaultProjectType & enableUsageTracking
• Added logic for each setting
• Added tooltip to project name - so the project path will be displayed as requested in Gitter. (Small addition so I thought I can add it here)

Things to discuss

• Should we auto-close on save button click. I think it's better to keep it open and display a short text, so it's visible that the settings are saved. No auto-closing - Check ProjectConfiguration later to be consistent.
• Do we need an automatic saving e.g. trigger with throttling after every change or save on modal close? Optional feature, maybe add later.

Todos:

• [x] Fix unit tests foranalytics.saga - Not sure how I can fix it. Fails with enableUsageTracking undefined. Do I need to have a state?
• [x] Fix project path selection bug. If user wants to pick another path during Create project it won't be set. Expected to temporarly use the selected path. At next modal open use default again.
• UI styling improvements:
  • [x] Gap ProjectType Selection and Privacy is too large.
  • [x] Duplicate text for DefaultProjectType
  • [x] Is activeField required here? Check how it looks at CreateProject modal.
• [x] Check flow in AppSettingsModal. Return type for produce - need to check what's required.
• [x] Remove commented lines

Screenshots/GIFs: Preferences modal

Tooltip for project directory

[melanieseltzer]

Great 👍 Kept my review light so far, since it's early WIP.

Should we auto-close on save button click. I think it's better to keep it open and display a short text, so it's visible that the settings are saved.

I think yes to keeping it open, as well. If a user hits save but realizes they forgot something, kinda annoying to have to bring it back up again. Sidenote - if we keep it open should we make the Project Settings modal behave the same way? Currently it closes the modal on save.

Do we need an automatic saving e.g. trigger with throttling after every change or save on modal close?

I think auto saving is traditionally used for stuff like saving drafts. I wonder if it's a bit overkill for a preferences modal where you aren't typing anything, just selecting buttons. I think I prefer a save button here, and that would keep consistent with Project Settings modal as well (I think consistency is important).

[Edit] Hm the more I think about it, the more I am open to auto save. Could be good with a flashing 'Saved!' on screen somewhere. Though I wonder if the modal should be that large if there would be no save button.

I think let's hear from others about this!

[AWolf81]

@melanieseltzer Thanks for your review of my WIP.

For auto-saving: I can't exactly say which is the right way but I think for native apps there is always a button to confirm or cancel a dialog. That's why we should stick to the save button. But auto-saving would be a nice option - maybe we could add this later as option to the app settings. So it could be enabled. Anyway I think we need a saved! flash so it's clear that everything is OK and yes we should be consistent with the Project configuration modal. (The auto closing is also there because it's sort of a confirmation that saving happend.) I'd like to add the Saved flash in a PR after app settings landed as it's an improvement.

Things I think we could do after this PR are:

• Saved flash feature & modify ProjectConfiguration to be consistent. I think for now it's OK to create a SaveButton that will add a saved flash close to the button. Later we could use a notification component but we don't have that in place yet.
• Refactor app settings rendering so it's automatically rendering the dialog based on the appSettings state. This will require more information on state e.g. optionType: (toggle | textInput | directoryInput | projectTypeSelection). Then we can map to the right component and render it - maybe some additional props for rendering are needed (I haven't checked).

[AWolf81]

@melanieseltzer Could you please review again? I've addressed everything.

Except the point from Mathieu. I think it would be good to have type checking for the mentioned function but I think it's not that critical as I think the rendering will be automated later.

For saved flash we should open an issue after this PR. So the user get's a feedback that it's saved. Also we'll modify the ProjectConfigurationModal saving to the same behviour for consistency. I'll create the issue and mention that we have to wait for this PR to be merged.

[AWolf81]

@melanieseltzer Thanks for your review.

I've addressed everything and also improved the UI (see updated screenshot in PR comment above). Sorry, for the left over commented lines that I missed to remove. They're removed.

Also the input can be modified by typing now. What do you think should we add a check if the folder exists on save of the new path? Or can we improve the creation so nested new folders are supported and only check that the path is valid? At the moment, the user can enter an invalid path and other scripts (e.g. CreateNewProject) will fail.

But I think we can improve this after this PR. If the user enters a new path that's one level below an existing that's working because it will be created but if it's nested it will fail with the following error in create-project.service line 62:

[melanieseltzer]

Also the input can be modified by typing now. What do you think should we add a check if the folder exists on save of the new path?

Good catch. This is something I never considered with my initial suggestion 🤔

I think if it's possible to break it right now, we shouldn't have it editable until we do those checks (let's open a fresh issue and refactor later).

This PR is getting long and I'm sure you're sick of these changes 😆, so I think let's make it inputEditable={false} for now to get this merged, and I don't mind going back to do the style changes myself since I'm being picky.

[AWolf81]

@melanieseltzer Oh, I've seen your approving comment too late. :-) So I did some extra work by adding your mentioned styling (see screenshot above) and I also added a path check. The path check can be improved later because it's allowed to enter a path where the last folder doesn't exists. For now we're checking the full path.

I think now it's good to go.

[melanieseltzer]

I think it looks good!

There's something funky going on with the check, though. If you enter a bad path and click save, it throws the dialog as expected. But then if you correct it, it still throws... forever. Will not let you save until you refresh the app.

Is it something we should create a separate issue for?

[AWolf81]

@melanieseltzer yes, please create an issue for it. I couldn't re-create it on Windows. Maybe it has to do something with the replacing of the first slash. Is it saving after initial render?

[melanieseltzer]

320 created. It's not saving for me on the initial render. But if I choose from the actual directory picker it works. It's just when typing the path that it's doing this.

If I have some time tomorrow/the weekend to look into it I will, but at least issue is created in case someone else wants to.