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

lk/grade implementation #67

Merged
merged 11 commits into from
Oct 30, 2023
Merged

lk/grade implementation #67

merged 11 commits into from
Oct 30, 2023

Conversation

leangseu-edx
Copy link
Contributor

@leangseu-edx leangseu-edx commented Oct 24, 2023

  • feat: implement grade route
image Screenshot 2023-10-24 at 1 00 30 PM

@muselesscreator
Copy link
Contributor

muselesscreator commented Oct 24, 2023

Peer assessments should be listed within the collapsible, not each get their own.
also, could we get a screenshot of an expanded assessment?

https://www.figma.com/file/bTYCd2VptQT5Apu2XcQPy0/ORA-MFE?type=design&node-id=445-15099&mode=design&t=28WU4BTEZwOmaiI5-0

@leangseu-edx
Copy link
Contributor Author

Peer assessments should be listed within the collapsible, not each get their own. also, could we get a screenshot of an expanded assessment?

https://www.figma.com/file/bTYCd2VptQT5Apu2XcQPy0/ORA-MFE?type=design&node-id=445-15099&mode=design&t=28WU4BTEZwOmaiI5-0

The problem with that is that peer have multiple grades and the figma expect a single peer.

@mattcarter
Copy link
Contributor

mattcarter commented Oct 25, 2023

Peer assessments should be listed within the collapsible, not each get their own. also, could we get a screenshot of an expanded assessment?
https://www.figma.com/file/bTYCd2VptQT5Apu2XcQPy0/ORA-MFE?type=design&node-id=445-15099&mode=design&t=28WU4BTEZwOmaiI5-0

The problem with that is that peer have multiple grades and the figma expect a single peer.

No, the figma references a multi-peer response. Reviewing the peer grade only mock, I see design references for at least 5 peers
https://www.figma.com/file/bTYCd2VptQT5Apu2XcQPy0/ORA-MFE?type=design&node-id=552-54495&mode=design&t=iKfHp5nOWtXmDWvh-4

Copy link
Contributor

@mattcarter mattcarter left a comment

Choose a reason for hiding this comment

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

Update makes sense for multiple peers. I approve, but would like @muselesscreator to check before you merge

@leangseu-edx leangseu-edx force-pushed the lk/grade-implementation branch from fb4cf6c to 2b82b78 Compare October 26, 2023 13:51
}
if (assessments.peer) {
finalStepScore = finalStepScore || assessments.peer.stepScore;
const stepName = formatMessage(messages.peerStep);
Copy link
Contributor

Choose a reason for hiding this comment

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

pls use stepLabel if you are talking about a message. Mostly because we have a global data object called stepNames and use that as a key in many places.

stepName={stepName}
stepScore={assessments.peerUnweighted.stepScore}
>
{assessments.peerUnweighted.assessment?.map((peer, index) => (
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be assessments for peer and peerUnweighted. should be reflected in both the components and the mock data.

const data = loadState({ view, progressKey });
const result = {
...camelCaseObject(data),
};
Copy link
Contributor

Choose a reason for hiding this comment

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

why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

needed to selective camelize earlier with selected option and forgot to change back.

<h5 className='mb-0'>{criterionName}</h5>
<InfoPopover onClick={() => {}}>
<p>
Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do
Copy link
Contributor

Choose a reason for hiding this comment

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

uuuuugh. we need to get these messages still

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, these messages should come from the ora config data, as in the ESG rubric

Copy link
Contributor

@muselesscreator muselesscreator left a comment

Choose a reason for hiding this comment

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

LGTM

@leangseu-edx leangseu-edx force-pushed the lk/grade-implementation branch from 966b2cb to d1a99d1 Compare October 30, 2023 13:26
@leangseu-edx leangseu-edx merged commit c0890f9 into master Oct 30, 2023
2 of 3 checks passed
@leangseu-edx leangseu-edx deleted the lk/grade-implementation branch October 30, 2023 13:27
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