Skip to content

Updating react-table to v7 #122

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

Merged
merged 33 commits into from
Feb 7, 2023
Merged

Updating react-table to v7 #122

merged 33 commits into from
Feb 7, 2023

Conversation

jakub-pomykala
Copy link
Member

@jakub-pomykala jakub-pomykala commented Jan 19, 2023

@jakub-pomykala
Copy link
Member Author

For the super-linter check the env option to VALIDATE_CSS is set to false but it was still running checks on the CSS with jscpd. I managed to remove the failure by setting CSS styling that was flagged as clone to multiple elements at the same time.

@gkwan-ibm
Copy link
Member

Also, there are two // tag::table[] showing in the table and better update the table screenshot in the "Try what you’ll build" section because the table bottom looks differently.

Screen Shot 2023-01-25 at 4 50 11 PM

@gkwan-ibm
Copy link
Member

gkwan-ibm commented Jan 25, 2023

At "Importing the HTTP client" subsection,

  • should remove the following statement?
    ...Next, add the componentDidMount() method to your component.

  • the hostspots not work
    ...to posts. The convertData function ...

  • no more convertData, right?
    "...You will notice the object spread syntax that the convertData ..."

  • no more this.setState , right?
    The this.setState function...

@gkwan-ibm
Copy link
Member

Failed to run mvn process-resources in the "Building and packaging the front end" section

[INFO] Creating an optimized production build...
[INFO] Failed to compile.
[INFO] 
[INFO] Attempted import error: './Components/App' does not contain a default export (imported as 'App').
[INFO] 
[INFO] 
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  01:28 min
[INFO] Finished at: 2023-01-25T17:20:00-05:00
[INFO] ------------------------------------------------------------------------
[ERROR] Failed to execute goal com.github.eirslett:frontend-maven-plugin:1.10.0:npm (npm run build) on project guide-rest-client-reactjs: Failed to run task: 'npm run build' failed. org.apache.commons.exec.ExecuteException: Process exited with an error: 1 (Exit value: 1) -> [Help 1]

@jakub-pomykala
Copy link
Member Author

In regards to this comment: I have run end to end and detected no issues.

@gkwan-ibm
Copy link
Member

  • better make the text in the screenshot bigger
  • in the "Try what you’ll build" section, after liberty:run, http://localhost:9080 does not show the table
  • should not have following files. It should be created by users
    • start/src/main/frontend/src/index.js
    • start/src/main/frontend/src/Components/App.js
    • start/src/main/frontend/src/Components/ArtistTable.js
  • in the "Importing the HTTP client" subsection,
    • the posts hotspot in "...changes by assigning response.data to posts." does not highlight any code.
    • the convertData hotspot in "The convertData function ...." seems highlight unrelated
  • in the "Building and packaging the front end" section, mvn process-resources successfully ran at the start directory but the http://localhost:9080 does not show the table

@jakub-pomykala
Copy link
Member Author

Changes done - but once again no issues running the app which straight away displays the table..

@gkwan-ibm
Copy link
Member

  • better make the text in the screenshot bigger
  • in the "Try what you’ll build" section, after liberty:run, http://localhost:9080 does not show the table
  • in the "Importing the HTTP client" subsection,
    • "The getArtistsInfo() function ..." and "...and the getArtistsInfo() function."
      • getArtistsInfo() does not match the code GetArtistsInfo
    • "...changes by assigning response.data to posts."
      • the posts hotspot does not highlight anything
    • "The convertData function ...."
      • although the convertData does not highlight anything, it cannot be found in the code. Should remove this statement or need to rewrite something else to explain the code.

@jakub-pomykala
Copy link
Member Author

Comment adressed in 866d616 and 78452b4

@gkwan-ibm
Copy link
Member

gkwan-ibm commented Feb 2, 2023

  • the image has // tag::tabe[] and it is even better if can make the width a bit shorter (from 1878px to 1440px), so that the image can be displayed bigger
  • in the "Importing the HTTP client" subsection,
    • "The convertData function ..." still not make sense.

Copy link
Contributor

@ReeceNana ReeceNana left a comment

Choose a reason for hiding this comment

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

Ran guide end-to-end. All tests passed, no errors.

Copy link
Member

@dmuelle dmuelle left a comment

Choose a reason for hiding this comment

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

ID review, let me know if you have any questions, thanks.

gkwan-ibm and others added 4 commits February 7, 2023 11:44
Co-authored-by: David Mueller <[email protected]>
Co-authored-by: David Mueller <[email protected]>
Co-authored-by: David Mueller <[email protected]>
Copy link
Member

@dmuelle dmuelle left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

@gkwan-ibm gkwan-ibm merged commit d96a3dd into staging Feb 7, 2023
gkwan-ibm added a commit that referenced this pull request Feb 9, 2023
Merge staging to prod - Updating react-table to v7  (#122)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Completed
Development

Successfully merging this pull request may close these issues.

4 participants