-
Notifications
You must be signed in to change notification settings - Fork 0
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
created front end part of admin portal #99
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code is quite neat and I like the way the page/component is currently structured. I think this is a good skeleton to build other logic upon. Great job!
</HeadingDiv> | ||
<PendingSection> | ||
<h3>Pending Requests</h3> | ||
<ListOfClubs columnTitles={columnTitles} clubs={clubs}></ListOfClubs> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
As a general rule of thumb, variables such as
clubs
andcolumnTitles
in this case should not be hardcoded and passed directly to<ListOfClubs />
. -
Instead, it should be passed in as props to
<Portal/>
and then to<ListOfClubs />
. -
The reason for this is that during the lifecycle of the Portal component, the clubs in the Pending section might change, such as being moved to the Approved Section or Trash Section. If it's passed directly to ListOfClubs, bypassing Portal, we won't be able to change it later based on user actions.
<Row> | ||
{columnTitles.map((title,columnIndex)=>{ | ||
return( | ||
<ColumnTitle key={columnIndex}>{title}</ColumnTitle> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding the key prop is absolutely essential when rendering a list of items, great job!
// const [moreInfo, setMoreInfo] = useState(false); | ||
const expandInfo = (index)=>{ | ||
var isDisplayed = document.getElementById(`more-info-${index}`).style.display; | ||
isDisplayed ? isDisplayed==="none" ? document.getElementById(`more-info-${index}`).style.display= "inherit" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Although in React we generally don't manipulate the DOM directly, I really like your approach here!
- For code clarity, it might be better to write it as follows:
/* Your logic
if ( isDisplayed ) {
if ( isDisplayed === "none" )
isDisplayed = "inherit";
else
isDisplayed = "none";
} else {
isDisplayed = "none";
}
*/
/* from above, it seems that it can be reduced to the following */
var isDisplayed = document.getElementById(`more-info-${index}`).style.display;
if ( isDisplayed === 'inherit')
document.getElementById(`more-info-${index}`).style.display = 'none';
else
document.getElementById(`more-info-${index}`).style.display = 'inherit'
Code has been merged in after the following processes:
|
I made this by creating a "Portal.js" file within the "containers" directory. I then created the main outline of the portal and created a component for showing the list of clubs. In that component, I just passed in data from the "portal.js" file to the "ListOfClubs.js" file. To open up more information about each club, I gave it a unique "id" as I iterated through the data and once one of those arrows gets clicked on, we take the id and pass it on to a function. This function then gets sets the display from "hidden" to "initial".