Peer review mac

[jimmy1wu]

Functionality

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

Formatting & Presentation

README.adoc checks:

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

pom.xml checks (if files are present):

• [ ] Check that the file follows the template found here: https://github.com/OpenLiberty/draft-guides-template/blob/master/finish/pom.xml
• [ ] 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

- [ ]  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>

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

Overall checks:
- [ ] Check the consistency of guide with the template and other guides
- [ ] Check the quality of code according to the best coding practices
- [ ] 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`)
- [ ] Check that the directories are properly structured
- [ ] 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). 
- [ ] Check the `attribution` statement is accurate for the guide
- [ ] Verify the `travisTest.sh` script, if any, is accurate and consistent with other guides

---

- [ ] Additional tests where applicable: 
    - [ ] Define test coverage and review with team (including guide contributor, if available)
    - [ ] Define detail test cases 
    - [ ] Consider corner cases targeting the specific guide
    - [ ] Consider corner cases UI tests
    - [ ] Consider testing URL on all browsers, ie, FF, Chrome, Safari
    - [ ] Consider testing the `curl` command for URL visits
- [ ] Consider building with both Maven and Gradle build tools 
- [ ] Testing with different IDEs, ie, Atom, Eclipse (Optional: VS.code, IntelliJ, Microclimate)
- [ ] Run Acrolinx Checker on draft (above 70 score approximately)
- [ ] 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
- [ ] Check the appearance of the guide on test site for the following items: 
    - [ ] Table of contents 
    - [ ] Headings
    - [ ] Paragraphs
    - [ ] code snippets
    - [ ] outputs
    - [ ] links
    - [ ] hotspots 
- [ ] Test the guide end-to-end with working instruction and sample code
- [ ] Perform all the defined test cases 

**General**
- [x] What is the usage of package-lock.json in `finish/draft-guide-rest-client-react/src`? can it be removed?
- [x] `react-router-dom` found in package.json can probably be removed. the app is not using react router api
- [x] I don't think you need `bootstrap` or `react-boostrap` packages. you are already import the react-table stylesheet for the table component
- [ ] Why did we decide to use axios? You can avoid adding additional dependency by using the Fetch API which is native to web browsers and doesn't require an additional package. Fetch is what is used in the react docs.
- [x] Fetchdata does not really make sense as a react component name. `ArtistTable` or something similar would be better.
- [x] state.posts should not be initialized as an empty object literal if it's being assigned as an array later. should be initialized as an empty array `[]`.
- [x] Random console.log https://github.com/OpenLiberty/draft-guide-rest-client-reactjs/blob/dev/finish/src/main/frontend/src/Components/Fetchdata.js#L37
- [x] getPosts is kind of a weird function name, doesn't really relate to what it is doing. should be called getArtists?
- [x] https://github.com/OpenLiberty/draft-guide-rest-client-reactjs/blob/dev/finish/src/main/frontend/src/Components/Fetchdata.js#L44 This is a very confusing function, especially with the single letter variable names. I assume this is unwinding the array in the response

**Project Configuration**
- [x] `The frontend of your application uses Node.js to compile your React code.` Not sure this is exactly correct. I think the react code is compiled using babel and bundled using webpack, which are preconfigured by CRA.
- [x] `It’s convenient package manager, npm, is used to compile and execute your React code` It's should be its. Also, i don't think npm is used to compile and execute react code. React is compiled by babel and executed in the browser. npm run build and npm run start just kick off scripts defined in the package.json to build and run the react app
- [x] `The frontend-maven-plugin is used to install Node.js in your working directory` i dont think the hotspots are is correct. `npm install` does not install node. it installs the dependencies listed in ur package.json from the npm registry into a folder called node_modules 

**Creating the default page**
- [x] `The index.js file should be the main entry point of the application as it coincides with the index.html file. This is where your code is always going to be run.` should probably hotspot the index.html file you're referring to. Also it would prob be good to explain how `ReactDOM.render(<App />, document.getElementById('root'));` relates to `<div id="root"></div>`. Also `index.js file should be the main` -> `index.js file is the main` since this is how CRA is configured and won't work otherwise
- [x] `React is evident through the Virtual DOM that it offers, which is essentially a copy of the browser DOM that resides in memory.` Should explain what the DOM is in this paragraph.
- [x] `The ReactDOM package prov` Package name should be react-dom, not ReactDOM.
- [x] `This file is invoked by in using create-react-app` __invoked by in__ does not seem right
- [x] Unnecessary to wrap `<Fetchdata />` with react fragment since it only has 1 child

**Creating the React components**
- [x] `To display the returned data, you will use the practice of pagination. ` The application does not have enough data to paginate. Max page size is 6 and we only have 4 rows after unwinding the albums. if you create your own table, you can remove the react-table dependency.
- [x] `<React.Fragment>` is used in a few places in your application. Should explain what it is or what it does
- [ ] did not explain defaultPageSize and pagesizeoptions table props. also, users might not know what a prop is
- [x] `The {posts} prop corresponds to the consumed data from the API endpoint and is assigned to the data of the table. The {column} prop.` {posts} prop -> data prop, {column} prop -> columns props
- [x] `The return statement adds the paginated table to your application.` Not exactly. It returns a react fragment. The component is added by writing `<Fetchdata />` in App.js 

**Importing The HTTP client**
- [ ] `Unlike frameworks such as Angular that have a built-in HTTP client, you need to use a third-party vendor HTTP client to render data from an API endpoint.` You can use fetch api which is native to browsers and is also promise based. do not need to install additional package to use fetch.
- [x] `You also need to verify that the component was updated properly. This is done through the componentDidMount() lifecycle method.` Updating and mounting are different parts in the component lifecycle

**Running the tests**
- [ ] `Similar to the purpose in using the Axios HTTP-client, React doesn’t have a built-in test runner` React can use fetch api which does not require an additional lib

[salmad3]

Thank you for the feedback, @jimmy1wu. Good points raised about the concepts.

• For compiling, yes, in this case because we are using create-react-app it would depend on Babel. The other sections will use your suggestions as well.

• Using Axios was more for reliability and seems to be preferred by developers after some research.

  • It also supports the api that is native to ES6.
  • For running tests, Jest comes with create-react-app and since we used create-react-app to configure the front end there is mention of it.

• For the "props", would you suggest just changing it to "properties" rather than going into detail?

  • I did not explain what the "defaultPageSize" and "pagesizeoptions" table props were as I did not want to go into too much about the table itself; just a high-level reference. Before, I had also explained the table headers, accessors, but we noticed it took away from the focus which should be: consuming a rest service. Though, still valid. What do you think?

[jimmy1wu]

I don't think you need bootstrap or react-boostrap packages. you are already import the react-table stylesheet for the table component

Was not addressed. If you remove them, all that is affected is the font of the artists web service title and it'll use the default font from CRA. bootstrap and react-bootstrap seem unnecessary

For the "props", would you suggest just changing it to "properties" rather than going into detail?

depends on the scope of the guide

I did not explain what the "defaultPageSize" and "pagesizeoptions" table props were as I did not want to go into too much about the table itself; just a high-level reference. Before, I had also explained the table headers, accessors, but we noticed it took away from the focus which should be: consuming a rest service. Though, still valid. What do you think?

It felt strange since you explained the first 2 props and then stopped.

I rewrote your convertData function below using the spread operator and i think it looks cleaner. you can use it if you agree. Also i found it weird that you declare your convertData function expression at the bottom, since it looked like you were calling it before it was declared. i suggest moving it up or just putting the logic in the callback.

axios('http://localhost:9080/artists')
      .then(response => {
        const artists = response.data;

        const posts = [];
        for (const artist of artists) {
          const { albums, ...rest } = artist;
          for (const album of albums) {
            posts.push({ ...rest, ...album });
          }
        };

        this.setState({
          posts,
          isLoading: false
        });
      })

[salmad3]

Updated to use the default font.

Was not addressed. If you remove them, all that is affected is the font of the artists web service title and it'll use the default font from CRA. bootstrap and react-bootstrap seem unnecessary

• I changed it to properties for now. I would say the scope of the guide is more focused on consuming the returned JSON and running the front end on the OL server as a packaged application and maybe not so much all the nitty gritty.

• Thanks for the suggestion, yes this is cleaner 👍

  I rewrote your convertData function below using the spread operator and i think it looks cleaner. you can use it if you agree. Also i found it weird that you declare your convertData function expression at the bottom, since it looked like you were calling it before it was declared. i suggest moving it up or just putting the logic in the callback.

[jimmy1wu]

feedback addressed. closing