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

Draft PR: react-js template for wxp on Toolkit #110

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

YueLi-MSFT
Copy link

Thank you for your pull request! Please provide the following information.


Change Description:

Describe what the PR is about. 
  1. Do these changes impact any npm scripts commands (in package.json)? (e.g., running 'npm run start')
    If Yes, briefly describe what is impacted.

  2. Do these changes impact VS Code debugging options (launch.json)?
    If Yes, briefly describe what is impacted.

  3. Do these changes impact template output? (e.g., add/remove file, update file location, update file contents)
    If Yes, briefly describe what is impacted.

  4. Do these changes impact documentation? (e.g., a tutorial on https://docs.microsoft.com/en-us/office/dev/add-ins/overview/office-add-ins)
    If Yes, briefly describe what is impacted.

If you answered yes to any of these please do the following:
> Include 'Rick-Kirkham' in the review
> Make sure the README file is correct

Validation/testing performed:

Describe manual testing done. 

Copy link
Contributor

@millerds millerds left a comment

Choose a reason for hiding this comment

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

This repo needs to continue to support all host for xml and not be converted to just json for your specific scenario. There are changes being made to the repo to support both json and xml and you should wait and base your changes of that.

I would also note that when generating projects we currently have the user pick hosts to start with. If you want to change from that either to add the option for all hosts or to replace host specific options with always generating for all hosts then some more thought need to go into the design of the template with others (documentation team owns the content of the project and Outlook team should have a say) such that it's not overly complicated and easy enough to trim down when developers don't want everything.

@millerds
Copy link
Contributor

We cannot merge changes into master that are not ready for production. (WXP support is not even in preview yet)

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.

None yet

2 participants