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

ReadMes overview page update #1199

Merged
merged 2 commits into from
Mar 15, 2021
Merged

ReadMes overview page update #1199

merged 2 commits into from
Mar 15, 2021

Conversation

IgorCATech
Copy link
Contributor

Signed-off-by: Igor Kazmyr 43951184+IgorCATech@users.noreply.github.com

Proposed changes

This PR aims to fix the landing page Readme. Currently, the Readme reflects the contents of the Zowe Explorer core readme (one of the packages). The idea is to have a high-level overview ReadMe that includes information about existing ReadMes in the repository.

Release Notes

Milestone:

1.13.x

Changelog:

Types of changes

What types of changes does your code introduce to Zowe Explorer?
Put an x in the boxes that apply

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Updates to Documentation or Tests (if none of the other choices apply)

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This checklist will be used as reference for both the contributor and the reviewer

  • I have read the CONTRIBUTOR GUIDANCE wiki
  • PR title follows Conventional Commits Guidelines
  • PR Description is included
  • gif or screenshot is included if visual changes are made
  • yarn workspace vscode-extension-for-zowe vscode:prepublish has been executed
  • All checks have passed (DCO, Jenkins and Code Coverage)
  • I have added unit test and it is passing
  • I have added integration test and it is passing
  • There is coverage for the code that I have added
  • I have tested it manually and there are no regressions found
  • I have added necessary documentation (if appropriate)
  • Any PR dependencies have been merged and published (if appropriate)

Further comments

If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...

@jellypuno
Copy link
Contributor

I think we need the following info in the landing page:

  • directory structure
  • requirements
  • getting started (as a contributor)
  • running tests
  • How to contribute (link to contributing guidelines)
  • license

Sample: https://github.com/kriasoft/nodejs-api-starter

Or maybe something simple like the COBOL course: https://github.com/openmainframeproject/cobol-programming-course

@IgorCATech
Copy link
Contributor Author

ok, so looks like we need not just the repo ReadMes overview but the entire repo overview that includes the bullets (maybe not all) suggested by @jellypuno Is that a correct understanding?

@IgorCATech
Copy link
Contributor Author

@jellypuno There are TODOs in the "readme update..." commit, that have to with the License section and How to Contribute section. I'd like to get your feedback on the idea suggestion in the Contribute section.
Also, I lack knowledge on how to articulate that License section. 2 versions are drafted but there's no certainty either of them is correct.

@IgorCATech IgorCATech marked this pull request as ready for review February 18, 2021 13:26
Copy link
Contributor

@katelynienaber katelynienaber left a comment

Choose a reason for hiding this comment

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

Heyoo @IgorCATech, a few comments...sorry if I'm being too nitpicky ✌️ 🌮

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated

- **Delete a profile**: Right-click a chosen profile and select **Delete Profile** to permanently delete the profile. The functionality deletes a profile from your `.zowe` folder.
{TODO Do we want to create some sort of labels that will indicate that 'external' help is needed/wanted? Like "help needed" or something like that.}
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can include a list of the labels that we use to mark issues people can contribute to?

Like help wanted I think is for this, or good first issue

Should probably confirm with other people on the squad to decide which labels are appropriate...

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 agree! So, I've added some information about those labels. Now, the question here is whether someone will keep an eye on those labels. So far none of the 2 labels has been added to any issue.
@jellypuno Let me know what you think about including information about these labels in the doc. If that's a good idea, I suggest then asking the squad members to check the current open issues and to add the abovementioned labels if applicable.

packages/zowe-explorer/README.md Outdated Show resolved Hide resolved
@IgorCATech
Copy link
Contributor Author

@jellypuno It's still not clear what text to include in the License section (Lines 83-89). Any suggestions as to who can know this? I guess, the worst-case scenario - we remove this section altogether.

Copy link
Contributor

@lauren-li lauren-li left a comment

Choose a reason for hiding this comment

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

I think this updated readme has a lot of good information in it! I made a couple suggestions to consider, but overall, I think it's a big improvement.

Thanks @IgorCATech for these updates!

README.md Outdated

![Save](/docs/images/ZE-safe-save.gif?raw=true "Save")
<br /><br />
## Build Locally
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this "Build Locally" section be moved under the Zowe Explorer package's readme? The instructions are specific to building that vsix file.

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 think the rationale behind having this section in this readme is to provide developers with an excerpt from the Dev's readme and include the link to the Dev's readme at the same time.
Actually, originally this section was part of the core ZE readme (package's readme now) but we decided to exclude it from there in order not to overload users with more options to install the extension.

Copy link
Contributor

Choose a reason for hiding this comment

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

Personally, I think it makes more sense to put this under the Zowe Explorer package's readme. However, if the squad prefers to keep it in the main repo readme, then perhaps we could add a line specifying that these instructions are just for building the Zowe Explorer VS Code extension .vsix file.

README.md Outdated

### Create a new PDS and a PDS member
Ensure that you have met the [software requirements](#requirements) before you can build and test your Zowe Explorer.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe remove "can" from this line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch!

README.md Outdated

[Zowe Explorer](https://github.com/zowe/community#zowe-explorer) is a sub-project of Zowe, focusing on modernizing mainframe experience. [Zowe](https://www.zowe.org/) is a project hosted by the [Open Mainframe Project](https://www.openmainframeproject.org/), a [Linux Foundation](https://www.linuxfoundation.org/) project.
Welcome to Zowe Explorer! This project is powered by Zowe, [Open Mainframe Project](https://www.openmainframeproject.org/).
Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer the wording: "Zowe, a project hosted by the Open Mainframe Project".

The format "Zowe, Open Mainframe Project" reminds me of the format people use when listing their address (e.g. City, State or City, Country).

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 think the emphasis here should be put on Zowe Explorer rather than Zowe :) Maybe, something like "Welcome to Zowe Explorer, a project hosted by Open Mainframe Project!"?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, that works for me.

@@ -66,6 +66,17 @@ Create a profile, review the sample use cases to familiarize yourself with the c

You can now use all the functionalities of the extension.

#### Profile Validation

Zowe Explorer includes the profile validation feature that helps to ensure that z/OSMF is accessible and ready for use. If a profile is valid, the profile is active and can be used. By default, the feature is automatically enabled. You can disable the feature by right-clicking on your profile and selecting the **Disable Validation for Profile** option. Alternatively, you can enable or disable the feature for all profiles in the VS Code settings.
Copy link
Contributor

@lauren-li lauren-li Feb 25, 2021

Choose a reason for hiding this comment

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

We should use a more general term instead of "z/OSMF" here. Perhaps:

"helps to ensure that the specified connection to z/OS is accessible and ready for use."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then I would suggest "helps to ensure that the specified connection to z/OS is successfully established and ready for use."

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good to me!

README.md Outdated
2. Open the **DATA SETS** bar.
3. Click the **Add Profile** button on the right of the **DATA SET** explorer bar.
4. Select the profile that you want to add to the view as illustrated by the following screen.
## Available Documentation
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we say we are linking to documentation here, it might be helpful to link directly to the readme part (i.e. anchor tag) of these repo package pages. For example:

And so on.

README.md Outdated
Your PDS member (or PS) is uploaded.

**Note:** If someone else has made changes to the PDS member (or PS) while you were editing it, you can merge your conflicts before uploading to the mainframe.
- Install [Yarn](https://yarnpkg.com/getting-started/install).
Copy link
Contributor

Choose a reason for hiding this comment

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

This section might be clearer if we group the client-side prerequisites together separately from the host prerequisites instead of mixing the two together. Node.js and Yarn will be needed locally on the client-side. The (current) middle bullet point about TSO/E, etc. is host-side.

README.md Outdated

- **Associate profiles**: You can create a secondary association by right-clicking the profile and selecting the **Associate profiles** icon. For more information, see [the Associate profiles section](https://docs.zowe.org/stable/user-guide/ze-profiles.html#associate-profile) in Zowe Docs.
{TODO I don't know exactly how to formulate this section exactly. See the following 2 versions. Something tells me, it's the second version but I'd rather check with someone who knows for sure}
Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately, I'm also not sure what to put in this section. Zowe CLI's repo doesn't appear to have a license section in its readme. @jellypuno Perhaps if we are all unsure, we can reach out to Zowe leadership for guidance on this?

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 was told to scrap this section.

@jellypuno jellypuno added this to the 1.13 Release milestone Feb 25, 2021
@lauren-li
Copy link
Contributor

For the Zowe Explorer package readme, I feel we should make the link to Zowe Docs' Zowe Explorer pages more obvious. Maybe we can put a "What's next?" section under the "Usage Tips" section, and say something like:

For more information about all the exciting things you can do with Zowe Explorer, including USS and Jobs interactions, check out our [full documentation](<Link to Zowe Docs' Zowe Explorer page>).

@IgorCATech
Copy link
Contributor Author

@lauren-li We have a note under the Usage section with a link to the configuration page (part of the install doc on the ZE-related Zowe Docs page), I think I should replace it with your suggestion to make users aware that there is more to Zowe Explorer than what's described in this readme 👍 Great idea!

@IgorCATech
Copy link
Contributor Author

I understand that there's a problem with running the Run yarn install --frozen-lockfile command in 2 builds. Can someone take a look and let me know how to address this issue, please?

@IgorCATech
Copy link
Contributor Author

Added a draft of the release note for 1.13. Can I ask @jellypuno and/or @lauren-li to review, please? Also, I lack information on the 'zftp extension for data set'. Is that a new feature in ZE or is that an extension for ZE, that introduces new features? Many thanks!

@lauren-li
Copy link
Contributor

Also, I lack information on the 'zftp extension for data set'. Is that a new feature in ZE or is that an extension for ZE, that introduces new features?

@IgorCATech This changelog item is actually for the Zowe Explorer FTP extension package. I don't think it fits on Zowe Explorer's Readme page. It allows the Zowe Explorer FTP extenion user to use their zFTP profile in Zowe Explorer's Data Sets view.

README.md Outdated
3. Open the profile and PDS containing the member.
4. Right-click on the PDS member that you want to delete and select **Delete Member**.
5. Confirm the deletion by clicking **Yes** on the drop-down menu.
Clone the repository and run `yarn workspace vscode-extension-for-zowe package` to build a VSIX file and start working with the extension.
Copy link
Contributor

Choose a reason for hiding this comment

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

I tried this command and I ended up with just one .vsix file. It is better to replace yarn workspace vscode-extension-for-zowe package to yarn run package. And after that, maybe it is good to mention that the vsix is in the dist folder

- Fixed the issue that caused removed favorite profiles to persist between IDE sessions.
- Fixed the issue that prevented updated credential prompting from occurring when a profile was marked “invalid”.
- Added the monorepo landing Readme that contains the high-level overview of the repository folders such as `packages` folder, instructions on how to contribute to the project and links to Medium articles providing additional useful information about Zowe Explorer and Zowe.
- Added the previously selected `Reject Unauthorized` value to the placeholder text of the entry field while updating an existing profile.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is actually a bug fix. It loads the current RejectUnauthorized value from your profile and also bring it to the top of the selection and highlight it.

katelynienaber
katelynienaber previously approved these changes Mar 9, 2021
Copy link
Contributor

@katelynienaber katelynienaber left a comment

Choose a reason for hiding this comment

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

Read through it again and it seems okay to me :) Can give a thumbs up 👍

jellypuno
jellypuno previously approved these changes Mar 10, 2021
Copy link
Contributor

@jellypuno jellypuno left a comment

Choose a reason for hiding this comment

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

Looks good to me!

Copy link
Contributor

@crawr crawr left a comment

Choose a reason for hiding this comment

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

Top level readme looks good. I just had some minor comments on the changelog and release notes.

packages/zowe-explorer/CHANGELOG.md Outdated Show resolved Hide resolved
packages/zowe-explorer/README.md Outdated Show resolved Hide resolved
packages/zowe-explorer/README.md Outdated Show resolved Hide resolved
packages/zowe-explorer/CHANGELOG.md Outdated Show resolved Hide resolved
@IgorCATech IgorCATech requested a review from crawr March 11, 2021 11:04
crawr
crawr previously approved these changes Mar 11, 2021
Copy link
Contributor

@crawr crawr left a comment

Choose a reason for hiding this comment

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

Thanks for addressing my comments @IgorCATech . LGTM

README.md Outdated

## Usage tips
[Zowe Explorer FTP Extension ReadMe](https://github.com/zowe/vscode-extension-for-zowe/blob/master/packages/zowe-explorer-ftp-extension/README.md) — contains information about how to install and use the Zowe Explorer extension for FTP. The extension adds the FTP protocol to Zowe Explorer, enabling you to use z/OS FTP Plug-in for Zowe CLI profiles to connect and interact with z/OS USS.
Copy link
Contributor

Choose a reason for hiding this comment

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

@IgorCATech Note: If PR #1219 is merged in, this line should be updated to say "interact with z/OS USS and MVS". (This update can be in a separate PR, though.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the heads-up, @lauren-li I think if we are sure that #1219 will be merged together with the ZE 1.13 release PR, I can update this line with #1235 If we are not so sure, let me update this information in another PR. How does that sound?

Copy link
Contributor

@lauren-li lauren-li Mar 11, 2021

Choose a reason for hiding this comment

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

@IgorCATech That would be perfect, thank you!

README.md Outdated
For more information, see [Changelog](https://marketplace.visualstudio.com/items/Zowe.vscode-extension-for-zowe/changelog).

## Prerequisites
Client-side prerequisites:
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add VS Code here, as well? Seems like a given, but at the same time, you really can't use Zowe Explorer without it. (Although, technically there are alternative options like VSCodium and Theia, too.)

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 wouldn't add 'VS Code' as a prerequisite because this ReadMe is part of Marketplace. Seems redundant to me. Otherwise, I would list VS Code in the pre-req section

Copy link
Contributor

Choose a reason for hiding this comment

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

@IgorCATech Some users may come to this page on GitHub outside of the VS Code context, so I think having it listed could be helpful in those cases. But you have a point, as well.

README.md Outdated
@@ -1,296 +1,94 @@
# Zowe Explorer
# Getting Started with Zowe Explorer
Copy link
Contributor

Choose a reason for hiding this comment

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

Simply "Zowe Explorer" seems like a better top-level header to me. Most other Zowe repos I've looked at (e.g. Zowe CLI, Zowe docs) just say the name of the project here. "Getting started" might be more fitting as a subheader.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe even "Zowe Explorer Overview"? In my understanding, this readme's goal is to provide users with a set of tools and a map of other readmes and useful Zowe Explorer-related links.

Copy link
Contributor

Choose a reason for hiding this comment

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

@IgorCATech That works for me. Thank you for explaining your thinking behind this!

README.md Outdated

**Note:** Alternatively, you can select 'No' to cancel the deletion.
```shell
git clone --origin=upstream --branch=main --single-branch https://github.com/zowe/vscode-extension-for-zowe.git
Copy link
Contributor

Choose a reason for hiding this comment

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

@IgorCATech We don't currently have a branch named main, so running this command will result in an error: warning: Could not find remote branch main to clone.. This command should probably be:

git clone --origin=upstream --branch=master --single-branch https://github.com/zowe/vscode-extension-for-zowe.git

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ooh.. they've renamed it back to 'master'. Good catch.

README.md Outdated
3. Open the profile and PDS containing the member.
4. Right-click on the PDS member that you want to delete and select **Delete Member**.
5. Confirm the deletion by clicking **Yes** on the drop-down menu.
1. Clone the repository by issuing the following command:
Copy link
Contributor

Choose a reason for hiding this comment

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

by issuing the following command:

Suggestion: "by issuing the following command in your local command line interface:"

(Just to be clear for those who are less familiar with this process.)

README.md Outdated

6. To delete a PDS, right-click the PDS and click **Delete PDS**, then confirm the deletion.
2. From your local copy of the repository, issue the following commands:
Copy link
Contributor

Choose a reason for hiding this comment

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

I would add a step above this to change directories into the newly-cloned repository. (Again, just to make it clear for those who are not familiar with the process.) Something like:

"Change directories into the newly-cloned repository: cd vscode-extension-for-zowe"

- Added the monorepo landing Readme that contains the high-level overview of the repository folders such as `packages` folder, instructions on how to contribute to the project and links to Medium articles providing additional useful information about Zowe Explorer and Zowe [#1199](https://github.com/zowe/vscode-extension-for-zowe/pull/1199). Thanks @IgorCATech
- Fixed the issue that prevented the list of recently opened files from being displayed upon request. You can access a list of recently opened files by pressing the Ctrl+Alt+R or Command+Alt+R key combination [#1208](https://github.com/zowe/vscode-extension-for-zowe/pull/#1208). Thanks @jellypuno
- Fixed the issue that prevented file picker from functioning. The file picker feature lets you filter your datasets in the tree by pressing the Ctrl+Alt+P combination [#992](https://github.com/zowe/vscode-extension-for-zowe/issues/992). Thanks @katelynienaber
- Fixed the issue that caused the content from a previously filtered directory instead of the currently filtered directory to be served [#1134](https://github.com/zowe/vscode-extension-for-zowe/issues/1134). Thanks @lauren-li
Copy link
Contributor

Choose a reason for hiding this comment

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

filtered directory

Change to: "filtered USS directory"

Comment on lines 44 to 46
- Fixed the issue that prevented the list of recently opened files from being displayed upon request. You can access a list of recently opened files by pressing the Ctrl+Alt+R or Command+Alt+R key combination.
- Fixed the issue that prevented file picker from functioning. The file picker feature lets you filter your datasets in the tree by pressing the Ctrl+Alt+P combination.
- Fixed the issue that caused the content from a previously filtered directory instead of the currently filtered directory to be served.
Copy link
Contributor

Choose a reason for hiding this comment

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

See my comments for the Zowe Explorer 1.13.0 changelog.


For more information about SCS, see [Secure Credential Store Plug-in for Zowe Explorer](https://docs.zowe.org/stable/user-guide/ze-profiles.html#enabling-secure-credential-store-with-zowe-explorer).

## Usage tips

- Use the **Add to Favorite** feature to permanently store chosen data sets, USS files, and jobs in the **Favorites** folder. Right-click on a data set, USS file or jobs and select **Add Favorite**.

- **Syntax Highlighting:** Zowe Explorer supports syntax highlighting for data sets. Fox example, you can use such extensions as [COBOL Language Support](https://marketplace.visualstudio.com/items?itemName=broadcomMFD.cobol-language-support) or [HLASM Language Support](https://marketplace.visualstudio.com/items?itemName=broadcomMFD.hlasm-language-support).
- **Syntax Highlighting:** Zowe Explorer supports syntax highlighting for data sets. You can search and install such extensions in VS Code Marketplace.
Copy link
Contributor

Choose a reason for hiding this comment

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

You can search and install

Change to: "You can search for and install"

@@ -339,7 +355,9 @@ For more information about SCS, see [Secure Credential Store Plug-in for Zowe Ex

- **Associate profiles**: You can create a secondary association by right-clicking the profile and selecting the **Associate profiles** icon. For more information, see [the Associate profiles section](https://docs.zowe.org/stable/user-guide/ze-profiles.html#associate-profile) in Zowe Docs.

For information on how to configure Zowe Explorer, see [Zowe Explorer Configuration](https://docs.zowe.org/stable/user-guide/ze-install.html#configuration).
- **Open recent members**: Zowe Explorer lets you open a list of members you worked on earlier. You can access the list by pressing Ctrl+Alt+R or Command+Alt+R.
Copy link
Contributor

Choose a reason for hiding this comment

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

Ctrl+Alt+R or Command+Alt+R

Change to: "Ctrl+Alt+R (Windows) or Command+Option+R (Mac)"

For information on how to configure Zowe Explorer, see [Zowe Explorer Configuration](https://docs.zowe.org/stable/user-guide/ze-install.html#configuration).
- **Open recent members**: Zowe Explorer lets you open a list of members you worked on earlier. You can access the list by pressing Ctrl+Alt+R or Command+Alt+R.

For the complete Zowe Explorer documentation that also includes information about USS and Jobs interactions, see [the complete Zowe Explorer documentation](https://docs.zowe.org/stable/user-guide/ze-install.html).
Copy link
Contributor

Choose a reason for hiding this comment

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

To avoid saying "the complete Zowe Explorer documentation" twice, we could phrase this as:

"For additional Zowe Explorer documentation that also includes information about USS and Jobs interactions, see the complete Zowe Explorer documentation."

@lauren-li
Copy link
Contributor

@IgorCATech I couldn't figure out how to retroactively add an overall comment with the review I just did, but in summary, I think this PR is shaping up well! I commented on some improvements I think we can make for the accuracy and clarity of our .vsix build/install steps, as well as some more minor corrections and suggestions. Thank you for your hard work on updating our documentation!

katelynienaber
katelynienaber previously approved these changes Mar 12, 2021
Copy link
Contributor

@katelynienaber katelynienaber left a comment

Choose a reason for hiding this comment

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

LGTM

@lauren-li
Copy link
Contributor

@IgorCATech I know my comments look like a lot, so if it would help you for me to make the edits straight into this PR or create a PR into yours, I would be happy to. Just let me know!

I tried to provide suggestions with all of my comments, so in most (if not all) cases, it should just be a matter of copy/pasting the suggestions you agree with.

Copy link
Contributor

@lauren-li lauren-li left a comment

Choose a reason for hiding this comment

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

This looks good to me now! Thank you @IgorCATech for these improvements and updates, and for being so responsive in working with me to address all the comments!

@jellypuno jellypuno self-requested a review March 12, 2021 18:46
jellypuno added a commit that referenced this pull request Mar 12, 2021
ReadMes overview page update
jellypuno and others added 2 commits March 12, 2021 14:09
ReadMes overview page update

Signed-off-by: zFernand0 <fernando.rijocedeno@broadcom.com>
@jellypuno jellypuno merged commit 9f28ee7 into master Mar 15, 2021
@jellypuno jellypuno deleted the T-level-Readme branch March 15, 2021 10:35
@IgorCATech IgorCATech restored the T-level-Readme branch March 15, 2021 10:58
@zFernand0 zFernand0 deleted the T-level-Readme branch February 11, 2022 12:17
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