Skip to content
This repository has been archived by the owner on Feb 9, 2024. It is now read-only.

London10 | Kristina Dubnyk | cyf-hotel-react | React #595

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

KristinaDudnyk
Copy link

No description provided.

@sonarcloud
Copy link

sonarcloud bot commented Jun 29, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

src/Bookings.js Outdated
};

useEffect(() => {
fetchData();
}, []);
Copy link

Choose a reason for hiding this comment

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

Nice use of dependencies array, empty means the logic inside the useEffect will be executed once.

// console.log("CustomerProfile props:", id);
const [clientData, setClientData] = useState(null);
const [loading, setLoading] = useState(false);
const [error, setError] = useState(false);
Copy link

Choose a reason for hiding this comment

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

Just a minor suggestion here, if the variable is a boolean, naming it as isLoading, setIsLoading, isError ... is more intuitive.

This reference is of Java but I think this convention is widely adapted
https://geosoft.no/javastyle.html#Specific

}
}, [id]);

return (
Copy link

Choose a reason for hiding this comment

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

Instead of deciding the rendering within return, do you thiink moving the condition out and returning accordlingly is more readable?

if(loading) {
  retrun (
    <div>Loading...</div>
  )
}

if(error) {
  return (
  <div>Error</div>
  )
}

return (
  <>
    ...
  </>
 )

<div className="footer">
<ul>
{location.map((element, index) => (
<li key={index}>{element}</li>
Copy link

Choose a reason for hiding this comment

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

Happy to see you have the key attribute for each li

reference of key https://legacy.reactjs.org/docs/lists-and-keys.html#keys

const [orders, setOrders] = useState(0);

function setOrdersFunction() {
setOrders(orders + 1);
Copy link

Choose a reason for hiding this comment

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

FYI there is another way to update the state setOrders( (order) => order + 1). in this case the result is the same.

}

function Order(props) {
const [orders, setOrders] = useState(0);
Copy link

Choose a reason for hiding this comment

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

if the variable is a number type rather than an array, it is better to name it order, orders implies it is an array.

setSelectedRows(selectedRows.filter((id) => id !== rowId));
} else {
// Row is not selected, add it to selectedRows
setSelectedRows([...selectedRows, rowId]);
Copy link

Choose a reason for hiding this comment

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

Nice use of Spread ... to update the state

@kitko112 kitko112 added the reviewed A volunteer has reviewed this PR label Jul 8, 2023
@sonarcloud
Copy link

sonarcloud bot commented Aug 11, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
review requested reviewed A volunteer has reviewed this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants