Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

User/ejohn/settings #121

Merged
merged 13 commits into from
Mar 28, 2023
Merged

User/ejohn/settings #121

merged 13 commits into from
Mar 28, 2023

Conversation

EricJohnson327
Copy link
Contributor

@EricJohnson327 EricJohnson327 commented Mar 28, 2023

Summary of the pull request

Add in depth Settings page.

References and relevant issues

Detailed description of the pull request / Additional comments

Here's is the detailed list of what is currently available in this Settings page change.

Top Level Page: Has 4 blades - Preferences, Accounts, Plugins, About
Preferences: Theme settings (light/dark/default)
Accounts: Login button and list of currently signed in accounts.
Plugins: List of all installed plugins as blades. Each blade has a toggle switch to enable/disable the plugin.
About: About info for Dev Home.

This to add in future:

  1. Notifications settings page. This already exists but is commented out. Can be enabled once notifications are plumbed through Dev Home.
  2. Clicking individual plugins in the Plugins settings page. This page will show the Settings Adaptive Card from the plugin.

Validation steps performed

PR checklist

  • Closes #xxx
  • Tests added/passed
  • Documentation updated

@@ -1,7 +1,7 @@
// Copyright (c) Microsoft Corporation and Contributors
// Licensed under the MIT license.

namespace DevHome.Core.Contracts.Services;
namespace DevHome.Common.Contracts;
Copy link
Member

@dkbennett dkbennett Mar 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we come up with something other than "Common"? That could be a grab bag of a whole bunch of stuff and is ambiguous. Is this common to all plugins? Common to DevHome? Common to Windows? Also why is common not under src? Is it not src? There's common\helpers and src\helpers, common\services and src services - what's the difference?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dev Home is split between core app and tools. "Common" is shared between the two. "src" is just the core application source code. Ideally, the "common" code would live in "src", but then we have a circular dependency.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would be fine with actually getting rid of the "common" namespace and just making all namespaces in DevHome.Common.* be DevHome.*, but that would definitely be a separate change.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dev Home is split between core app and tools. "Common" is shared between the two. "src" is just the core application source code. Ideally, the "common" code would live in "src", but then we have a circular dependency.

I would expect all source for shipping code to be under source, and if it's just the core application then that should be refactored into a sub folder of src for the app. Or change 'src' to be indicative that it is the app and not have a src folder.

@vineeththomasalex
Copy link
Contributor

Looks good to me. There will be some conflicts with #120. Let me know if you need help.

@EricJohnson327 EricJohnson327 marked this pull request as ready for review March 28, 2023 18:23
@EricJohnson327
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@EricJohnson327
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@EricJohnson327 EricJohnson327 merged commit 2e3ecc5 into main Mar 28, 2023
@crutkas crutkas deleted the user/ejohn/settings branch May 18, 2023 20:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants