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

Question sources #11658

Merged
merged 17 commits into from
Jan 3, 2024
Merged

Conversation

AllanOXDi
Copy link
Member

@AllanOXDi AllanOXDi commented Dec 17, 2023

Summary

This PR updates the examViewer module and ExamPage to accommodate the changes in the exam schema, ensuring consistent data structure representation for quizzes with version 3.

Closes #11581

References

#11581

Reviewer guidance

See this figma design

Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical user journeys are covered by Gherkin stories
  • Critical and brittle code paths are covered by unit tests

PR process

  • PR has the correct target branch and milestone
  • PR has 'needs review' or 'work-in-progress' label
  • If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • If this is an important user-facing change, PR or related issue has a 'changelog' label
  • If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

@github-actions github-actions bot added APP: Learn Re: Learn App (content, quizzes, lessons, etc.) DEV: frontend SIZE: medium labels Dec 17, 2023
@@ -0,0 +1,8 @@
*out
Copy link
Member

Choose a reason for hiding this comment

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

Not sure what .trunk is but you can add it to the .gitignore file in the Kolibri project root.

Once you do that you'll need to run git rm -r --cached .trunk to remove it from the git index. It will keep the files on your local machine, however. Relevant StackOverflow

@AllanOXDi AllanOXDi marked this pull request as ready for review December 22, 2023 19:35
Copy link
Member

@nucleogenesis nucleogenesis left a comment

Choose a reason for hiding this comment

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

In addition to some blocking comments, one other issue I noted is the spacing on the section title / description is a little off.

I think maybe we'll need ~1em padding so that the title isn't so close to the edge of its container:

image

Also, the chevron & section title area should be clickable such that it toggles the visibility of the description -- basically just like the accordion work from before but just a bit different.

In any case -- we also need to ensure we use the proper a11y properties like we did in the accordion like aria-expanded and the proper aria-labelledby / aria-label.

let allExerciseIds = [];
if (exam.data_version == 3) {
allExerciseIds = exam.question_sources.reduce((acc, section) => {
console.log(section);
Copy link
Member

Choose a reason for hiding this comment

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

Stray console.log

Comment on lines 102 to 103
(exam.question_sources = question_sources),
store.commit('examViewer/SET_STATE', {
Copy link
Member

Choose a reason for hiding this comment

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

These two lines seem odd like this -- should this instead be two separate lines like:

exam.question_sources = question_sources;
store.commit('examViewer/SET_STATE', {

:layout8="{ span: 2 }"
:layout4="{ span: 1 }"
>
<div style="float:right">
Copy link
Member

Choose a reason for hiding this comment

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

For this chevron you should be able to use text-align: right here rather than the float property. We typically avoid float because it is not intended for use as a general layout property but more to position things within areas of text.

Comment on lines 168 to 169
<div class="" style="float:right">
<div class="" style="float:right">
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering why the two floats are needed here.

In this case I think that we could leverage some other CSS properties for a cleaner and more consistent alignment & layout.

By using the position: relative; property on the parent element (the outermost <div> here), we can then use position: absolute; on any child of that div so that you can position that child w/ the right/left/top/bottom CSS properties

In this case, I'd suggest trying the following structure -- which may not be exactly right but should hopefully demonstrate the idea:

<KGridItem ...>
    <div style="position: relative;"> 
        <KButton style="position: absolute; right: 0">

This way it can be reasoned about kinda like:

The KButton is absolutely 0px from the right edge, relative to the parent div

Comment on lines 99 to 100
section_title: 'Section one',
description: 'Sample description',
Copy link
Member

Choose a reason for hiding this comment

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

One thing to note is that we should probably revert this. It is causing the tests to fail and if we are going to introduce a title and/or description by default we'll need to use a translated string and account for it in the tests. We can handle this in a separate follow-up issue

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops! I think I missed it before committing.

Copy link
Member

@nucleogenesis nucleogenesis left a comment

Choose a reason for hiding this comment

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

In testing things look good - I think that one thing we may need to work out is how we'll handle the empty state, but that can be handled in a follow-up issue. This appears to successfully make the new quiz structure backward compatible as needed.

When all the checks are passing, this is ready to merge. Thanks @AllanOXDi

@AllanOXDi AllanOXDi merged commit 63d2f8d into learningequality:develop Jan 3, 2024
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
APP: Learn Re: Learn App (content, quizzes, lessons, etc.) DEV: frontend SIZE: large SIZE: medium TODO: needs review Waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants