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

Enable Result to display different status icon #109

Closed
wants to merge 3 commits into from

Conversation

uhtna
Copy link

@uhtna uhtna commented Nov 30, 2021

I think I did it properly this time. I'm still trying to understand git.

@VulpesVulpes825 VulpesVulpes825 self-requested a review November 30, 2021 07:40
Copy link
Collaborator

@VulpesVulpes825 VulpesVulpes825 left a comment

Choose a reason for hiding this comment

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

In short, this does not work, as comp never been called again after initializing. I strongly suggest you to read the react tutorial before continuing.
Also, please run yarn prettier in client folder to clean up your style.

<div
style={{
const Result = function Result(status, title, intro) {
let comp;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Comp is never used in rendering.


if ( { status } === "success" ) {
comp = (
<div style={{
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please do not loop through things that is not changed.


return (
<div>
{ status, title, intro }
Copy link
Collaborator

Choose a reason for hiding this comment

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

You are simply displaying the props.

Copy link
Author

@uhtna uhtna Nov 30, 2021

Choose a reason for hiding this comment

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

Yeah this was from me just trying to figure out and understand the syntax of Return. This was just a testing measure and will be changed.

Copy link
Collaborator

@VulpesVulpes825 VulpesVulpes825 Nov 30, 2021

Choose a reason for hiding this comment

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

You have marked the pull request as ready to review, but the change break a functional component.

I am worrying, since this is not something that you still need to figuring out after four weeks since initial task assignment.

@VulpesVulpes825 VulpesVulpes825 changed the title Update index.jsx Enable Result to display different status icon Nov 30, 2021
@VulpesVulpes825 VulpesVulpes825 linked an issue Nov 30, 2021 that may be closed by this pull request
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.

Improve Result Component
2 participants