-
-
Notifications
You must be signed in to change notification settings - Fork 121
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
Updated details dashboard logic to avoid merging 3 APIs and utilize f… #488
base: main
Are you sure you want to change the base?
Conversation
…untions instead. Updated snapshot tests to accomodate new props associated with details dashboard ammendment. Minor name changes to old variables to be more specific Co-authored-by: Guillermo Flores V <[email protected]> Co-authored-by: Komal Kaur <[email protected]> Co-authored-by: Utsab Saha <[email protected]> Co-authored-by: Edgar Peralta <https://github.com/EPeralta18>
const handleShowDetails = () => { | ||
if (hideDetails) { | ||
setHideDetails(false); | ||
} else { | ||
setHideDetails(true); | ||
} |
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.
nit: I think we can reduce this to
const handleShowDetails = () => { setHideDetails(!hideDetails) } ...
just to shorten the function a little bit
certificationNumbers.fccCertifications | ||
); | ||
|
||
let superBlockJsons = await getSuperBlockJsons(superblockURLS); // this is an array of urls |
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.
Instead of a comment letting us know this is an array of URLs, could we make this variable more descriptive so that it describes what it is supposed to be?
let superBlockJsons = await getSuperBlockJsons(superblockURLS); | ||
let dashboardObjs = createDashboardObject(superBlockJsons); | ||
let totalChallenges = getTotalChallenges(dashboardObjs); | ||
let superBlockJsons = await getSuperBlockJsons(superblockURLS); // this is an array of urls |
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.
Instead of a comment letting us know this is an array of URLs, could we make this variable more descriptive so that it describes what it is supposed to be?
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.
Hi @GuillermoFloresV this name choice was introduced prior to me joining FCC which is why I added the comment. I could also make a separate 'easy' issue for this for CTI students or update it, @utsab let me know what you would prefer.
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.
{props.superblocksDetailsJSONArray.map((arrayOfBlockObjs, idx) => { | ||
let index = props.superblocksDetailsJSONArray.indexOf(arrayOfBlockObjs); | ||
let superblockDashedName = | ||
props.superblocksDetailsJSONArray[index][0].superblock; | ||
let progressInBlocks = superblockProgress(superblockDashedName); | ||
let superblockTitle = printSuperblockTitle(arrayOfBlockObjs); | ||
return ( | ||
<div key={idx} className={styles.board_container}> | ||
<DetailsDashboardList | ||
superblockTitle={superblockTitle} | ||
blockData={arrayOfBlockObjs} | ||
studentProgressInBlocks={progressInBlocks} | ||
></DetailsDashboardList> | ||
</div> | ||
); | ||
})} |
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.
Is there an edge case here where if we do not have a superblocksDetailsJSONArray
is undefined, or of length 0? We can add conditional rendering to check if our superblocksDetailsJSONArray
is undefined/has a length of 0, display a message to alert the user that there is no data to display
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.
Hi @GuillermoFloresV this is a good separate issue for someone wanting to contribute to the UI/Design side of the page. I prefer to create a separate issue for conditional rendering.
…untions instead. Updated snapshot tests to accomodate new props associated with details dashboard ammendment. Minor name changes to old variables to be more specific
Checklist:
Update index.md
)