Peer Review - Windows - Githubissues

TiagoF99 commented 4 years ago

Peer Review conducted on 3 platforms: Mac, Windows & Linux

Mac OS is our base OS as the dev content team generally develops on the Mac OS.
Conduct three separate peer reviews for each OS.

Peer Review: review to be done by a peer member.

Functionality

[x] Check that the guide has working instructions and sample code
[x] If any URL visits, try them on different browsers: Firefox, Chrome, Safari
[x] If any URL visits, try curl command where applicable

Formatting & Presentation

README.adoc checks:

[x] Ensure that command substitutions use the $(cmd) format and not `cmd`
[x] Ensure that the console output of the integrated tests do not include the [INFO] tab
[x] Check the quality and presentation of guide according to Structure and Style Guideline
[x] Check that the pom.xml, server.xml, etc files are clean
- [x] Remove any dependencies and features that are not required
- [x] Ensure that the parent pom.xml files do not contain any dependencies, and all dependencies are specified in the children pom.xml files
[x] Examples with the right outcomes are provided
[x] Example with the wrong outcomes, if any, are provided

pom.xml checks (if files are present):

[x] Check that the file follows the template found here: https://github.com/OpenLiberty/draft-guides-template/blob/master/finish/pom.xml

[x] Check that the following dependencies are included in all of the pom.xml files:


<!-- Provided dependencies -->
<dependency>
<groupId>jakarta.platform</groupId>
<artifactId>jakarta.jakartaee-api</artifactId>
<version>8.0.0</version>
<scope>provided</scope>
</dependency>

org.eclipse.microprofilemicroprofile 3.3 pom provided

- [x]  Check that the versions of plugins are directly provided and not fed in by a file property.
  - Not formatted like so:

<properties>
    <!-- Plugins -->
    <version.liberty-maven-plugin>3.2</version.liberty-maven-plugin>
</properties>

<!-- Liberty plugin -->
<plugin>
    <groupId>io.openliberty.tools</groupId>
    <artifactId>liberty-maven-plugin</artifactId>
    <version>${version.liberty-maven-plugin}</version>
</plugin>

  - Should be formatted like so:

<!-- Liberty plugin -->
<plugin>
    <groupId>io.openliberty.tools</groupId>
    <artifactId>liberty-maven-plugin</artifactId>
    <version>3.2</version>
</plugin>


- [x] Check that there are 4 spaces per indent for proper formatting
- [x] Ensure that the guide is using the latest version of the `liberty-maven-plugin` or `liberty-gradle-plugin` where applicable

Overall checks:
- [x] Check the consistency of guide with the template and other guides
- [x] Check the quality of code according to the best coding practices
- [x] Check that all licensing statements are properly stated in all files, with the correct year (Should be present in all Java files + the `index.html`)
- [x] Check that the directories are properly structured
- [x] Check that some of these `page-tags` are used in a guide: `MicroProfile, Maven, Docker, Kubernetes, Gradle, Java EE, Security, Cloud`. Only these tags are visible on the website. Latest list [here](https://github.com/OpenLiberty/openliberty.io/blob/master/src/main/content/_includes/visible-tags.liquid). 
- [x] Check the `attribution` statement is accurate for the guide
- [x] Verify the `travisTest.sh` script, if any, is accurate and consistent with other guides

---

Guide’s contributor’s (if available, otherwise peer tester’s) responsibility: 
- [x] Additional tests where applicable: 
    - [x] Define test coverage and review with team (including guide contributor, if available)
    - [x] Define detail test cases 
    - [x] Consider corner cases targeting the specific guide
    - [x] Consider corner cases UI tests
    - [x] Consider testing URL on all browsers, ie, FF, Chrome, Safari
    - [x] Consider testing the `curl` command for URL visits
- [x] Consider building with both Maven and Gradle build tools 
- [x] Testing with different IDEs, ie, Atom, Eclipse (Optional: VS.code, IntelliJ, Microclimate)
- [ ] Run Acrolinx Checker on draft (above 70 score approximately)
- [x] Consider SEO title and description for the guides 
- [ ] Ensure automated test is enable, set up with Travis CI, and able to schedule tests 
- [ ] Run `diff -r start/ finish/` and there's no differences
- [ ] Ensure that the automation tests are able to run when PR is created

Peer Tester’s responsibility: 
- [x] Check the appearance of the guide on test site for the following items: 
    - [x] Table of contents 
    - [x] Headings
    - [x] Paragraphs
    - [x] code snippets
    - [x] outputs
    - [x] links
    - [x] hotspots 
- [x] Test the guide end-to-end with working instruction and sample code
- [x] Perform all the defined test cases

TiagoF99 commented 4 years ago

What Youll Learn

[x] Seems redundant to highlight ReactJS twice and also to not highlight it in the other paragraphs before and after. Makes it inconsistent.
[ ] This is just the way the guide workflow was decided, but it might have been useful to actually show in the guide how to install react as an extension of https://openliberty.io/guides/rest-intro.html. I think this isn't really extendable as someone who already started an open liberty project and who wants to install ReactJS into it will now need to start a whole new project with the template given in this guide.

Getting Started

[x] It seems redundant to include Give it a try before you proceed. as the title already says the same thing as well as the following paragraph which gives instructions on how to start it
[ ] What is the purpose of ...and the static files from the production build can be found in src/main/webapp/static.? What Static files? why do I need to know where they are? etc. Seems confusing
[x] Might make it helpful to explicitly direct them to the finish directory.

Starting the service

[x] Remove the comma after "you" in Note: all the other dependencies that are installed in the project will show up in src/main/frontend/src/package.json that have already been generated for you, through...
[x] There are also pre-coded stylesheet files that can be found in src/main/frontend/src/Styles -> Maybe specify that the stylesheets are .css files?

Creating the default page

[x] Remove the duplicate "the": ...Here, you’re rendering the the App element on a DOM...
[x] Sentence doesn't make sense: This file is invoked by in using create-react-app

Creating the React components

[x] You will need to create the App.js file, which acts as a container for all other components.. Might be better to write something like: You will need to create the App.js file to act as a container for all the other components.
[x] might be useful to explain the ComponentDidMount() and that it runs when the component is rendered
[x] Seems redundant to highlight Axios multiple times.

Code

[x] in frontend/src you have the Styles and Components directories but it might be better to have them start with lower case letters to be consistent with other directories. This requires changes to the readme as well.
[ ] some of the licensing tags can be updated with 2017, 2020 in some java files and possibly others
[ ] no page-tags are used in the readme.adoc
[ ] add licensing tag to src/main/frontend/public/index.html maybe?
[x] possibly consider removing the .png files from src/main/frontend/public

salmad3 commented 4 years ago

Thank you for the feedback, @TiagoF99.

In terms of the guide being extendable to guide-rest-client-intro, I see what you mean. Though, I'd say the idea is to have that guide is more for reference, and the backend application is kept the same to be consistent.
The static files are the files that are produced "statically" from the frontend production build. Because the frontend is seperated from the backend, they need to be served to the OL server, and that is why we have a Project Configuration section to address this.
In general, static content would be any content that can be delivered to an end user without having to be generated.
For the licensing in the java files, they should stay at 2017 since no changes were made to the application
I don't think we would need a licensing tag for src/main/frontend/public/index.html because it does not "belong" to the "Open Liberty" configuration.
For this guide as it stands, I don't think any of the tags would apply. Even so, it would be added to the guide_tags.json in the guides-common repo.

TiagoF99 commented 4 years ago

New changes look good.

OpenLiberty / guide-rest-client-reactjs

Peer Review - Windows #6

Peer Review conducted on 3 platforms: Mac, Windows & Linux

Functionality

Formatting & Presentation

In terms of the guide being extendable to `guide-rest-client-intro`, I see what you mean. Though, I'd say the idea is to have that guide is more for reference, and the backend application is kept the same to be consistent.

OpenLiberty / guide-rest-client-reactjs

Peer Review - Windows #6

Peer Review conducted on 3 platforms: Mac, Windows & Linux

Functionality

Formatting & Presentation

In terms of the guide being extendable to guide-rest-client-intro, I see what you mean. Though, I'd say the idea is to have that guide is more for reference, and the backend application is kept the same to be consistent.

In terms of the guide being extendable to `guide-rest-client-intro`, I see what you mean. Though, I'd say the idea is to have that guide is more for reference, and the backend application is kept the same to be consistent.