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

feat: implement marking page scrollspy #34

Merged
merged 18 commits into from
Sep 12, 2024

Conversation

procaconsul
Copy link
Contributor

@procaconsul procaconsul commented Sep 10, 2024

Reproduce scrollspy capability from Answerbook v1 plus slicker makeup.

demo-scrollspy.mov

@@ -20,7 +21,6 @@ interface MarkInputPanelProps {
onSave: (_: MarkRoot) => void
}

const NO_MARK = 'No mark'
const NO_FEEDBACK = 'No comment'

Choose a reason for hiding this comment

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

Should NO_FEEDBACK also be grouped in ../constants?

Choose a reason for hiding this comment

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

Agreed

Copy link
Contributor Author

@procaconsul procaconsul Sep 12, 2024

Choose a reason for hiding this comment

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

I moved NO_MARK to constants because it's used in two different places now. NO_FEEDBACK is only used in this component.

id: string
active: boolean
label: string
partial?: number

Choose a reason for hiding this comment

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

Rename to partMarks and totalMarks for more clarity?

Copy link
Contributor Author

@procaconsul procaconsul Sep 12, 2024

Choose a reason for hiding this comment

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

This is a good suggestion but the ScrollspyItem is used for both Question and Section. For question, this is partial compound marks over total/available marks, for a section this is current mark over maximum mark. To avoid inconsistency, I have generalised by a single step and made this into a more generic indicator of partial/total. Don't have a strooong opinion on it though.

src/pages/MarkingPage/Scrollspy/index.tsx Outdated Show resolved Hide resolved
src/pages/MarkingPage/Scrollspy/index.tsx Outdated Show resolved Hide resolved
src/pages/MarkingPage/index.tsx Show resolved Hide resolved
},
{
rootMargin: '-10% 0px -80% 0px', // Adjusting to trigger around 3/4 up the viewport
threshold: 0.15, // Trigger when the element is fully visible in the viewport

Choose a reason for hiding this comment

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

@IlliaDerevianko This was very tricky to get right i.e. rootMargin and threshold were interacting with each other.

@@ -20,7 +21,6 @@ interface MarkInputPanelProps {
onSave: (_: MarkRoot) => void
}

const NO_MARK = 'No mark'
const NO_FEEDBACK = 'No comment'

Choose a reason for hiding this comment

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

Agreed

@sergiomaffeis
Copy link

all great, only one small issue:
why is Q2.a.ii marked as gray in the rightmost column, instead of green?

  • the mark box is green in the central part of the screen, there are both grade and comment (from automarker)
  • see video at 03 seconds

@sergiomaffeis
Copy link

ooops was i not supposed to close?

@sergiomaffeis sergiomaffeis reopened this Sep 11, 2024
@procaconsul
Copy link
Contributor Author

@sergiomaffeis You are very right. The implementation marks as RED the absence of marks, green the absence of positive marks, and grey the 0s. This was my understanding, but I may have misinterpreted the colour-coding. What should gray mean? Or, equivalently (as far as we are concerned), what did it use to mean?

@sergiomaffeis
Copy link

how about:

  • red: unmarked
  • green: marked
  • gray: questions contains marked/unmarked parts

@procaconsul procaconsul merged commit ce303e1 into master Sep 12, 2024
4 checks passed
@procaconsul procaconsul deleted the feat-implement-marking-page-sidebars branch September 12, 2024 15:15
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.

4 participants