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

Updated data processing and added initial details dashboard. #476

Closed
wants to merge 6 commits into from

Conversation

ngillux
Copy link
Contributor

@ngillux ngillux commented Sep 6, 2023

Checklist:

@ngillux ngillux requested a review from a team as a code owner September 6, 2023 23:03
@utsab
Copy link
Collaborator

utsab commented Sep 11, 2023

@ngillux After merging PR #473, the current PR is showing merge conflicts in two files. Can you take a look at the merge conflicts and resolve them?

@ngillux
Copy link
Contributor Author

ngillux commented Sep 11, 2023

@utsab I resolved the merge conflict.

In the api_processor.js file in the formattedStudentData() function I had changed a property name (changed 'name' to 'blockName') and added a property to the getSuperBlockJsons(superblockURLS) function which I thought would be needed for the details page in this PR. You should be able to see/test the updated changes in a Codespace now.

Let me know if there's any issues, or if you have any optimization suggestions.

Copy link
Contributor

@Komal914 Komal914 left a comment

Choose a reason for hiding this comment

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

Let's avoid merging superblocks with student progress blocks, and just update the UI. Should increase optimization and maintain the functionality.

let dashboardObjs = createDashboardObject(superBlockJsons);

// console.log('DDASHBOARD OBJS ===>', dashboardObjs)
let studentData = await getIndividualStudentData(studentEmail);
Copy link
Contributor

Choose a reason for hiding this comment

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

ISR should be used for the student data API response. This will allow us to avoid merging the data, and simply update the respective UI elements.

let mergedDashboardData = [];

// reshape data to pass into UI
allCurriculumData.forEach(superblockData => {
Copy link
Contributor

@Komal914 Komal914 Sep 28, 2023

Choose a reason for hiding this comment

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

lines 13-35: Let's remove this functionality to API processor an a function we utilize inside this file. Something like: updatedAllStudentCurriculumData(email). Then we can call this function and add the arguement for email inside this file. This should also prevent us from passing props (lines 129-132) inside Details Board from ./dashboard/v2/details/[id]/[studentemail].js

</div>

<DetailsBoard
studentCurriculumData={studentCurriculumData}
Copy link
Contributor

Choose a reason for hiding this comment

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

we no longer need to pass props here since this is being imported from API processor

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We will still need props here because getServerSideProps() is what gives us access to data stored on the server, which is needed as the reference/parameters for the utility functions in api_processor.js. Even though functions from api_processr.js can be called in the other components, it is still necessary to fetch the data on the server first - and we need getServerSideProps() + context.params for this.

@ngillux ngillux closed this Sep 30, 2023
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.

3 participants