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.

Sign up for GitHub

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

Jump to bottom

Questionnaire: Drag and Drop "Sorting" question #440

Open
wants to merge 12 commits into
base: MOODLE_400_STABLE
Choose a base branch
from
Open

Questionnaire: Drag and Drop "Sorting" question #440

wants to merge 12 commits into from

Conversation

lamtranb
Copy link
Contributor

@lamtranb lamtranb commented Dec 9, 2022

Hi,

We are from the Open University and would like to do this change:

Create a new question type Drag and Drop "Sorting".

Could you please review this commit and inform for any issue?

Thanks.

@mchurchward mchurchward changed the base branch from main to MOODLE_400_STABLE December 21, 2022 14:50
@lamtranb lamtranb force-pushed the wip323936 branch 2 times, most recently from 6556148 to 9fdb422 Compare January 1, 2023 12:51
@lamtranb
Copy link
Contributor Author

lamtranb commented Jan 6, 2023

Hi @mchurchward

I have rebased the code. Could you review this PR?

Thanks.

@mchurchward mchurchward self-requested a review January 30, 2023 18:47
mchurchward
mchurchward reviewed Jan 30, 2023
View reviewed changes
Copy link
Contributor

@mchurchward mchurchward left a comment

Choose a reason for hiding this comment

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

Hi. Thanks for this... some review comments:

  1. Is the new response table necessary? It looks like it could use the standard "questionnaire_response_text" table.
  2. What is the purpose of the new "mobile_questionnaire_init" function in the mobile.php class?
  3. Can you explain the changes made to "db/mobile.php"?
  4. It appears you have replaced the "styles_app.css" with the new "mobile_css.php" file. Given that, should the "styles_app.css" be deleted?
  5. Can the "questionnaire_prep_answer_sort_question" function in locallib be refactored into one of the classes? Maybe the specific response class?
  6. Can you add the new question type to the unit test for csv exporting?

@lamtranb lamtranb force-pushed the wip323936 branch 10 times, most recently from a5ff9a1 to f2591b0 Compare February 27, 2023 07:22
@lamtranb lamtranb force-pushed the wip323936 branch 12 times, most recently from c3b22b7 to e6f0e4d Compare March 22, 2023 11:48
@lamtranb
Copy link
Contributor Author

Hi @mchurchward, 

I updated the code.

  1. Is the new response table necessary? It looks like it could use the standard "questionnaire_response_text" table.

Yes, because we store a string example "1,2,3" which are sorting choices. And later we can transform it into array, so it has different behavior from questionnaire_response_text.

  1. What is the purpose of the new "mobile_questionnaire_init" function in the mobile.php class?

This is used for mobile app. It called the JS file: appjs/latest/mobile_view_activity.js. And sadly, we did not support for ionic 3 yet.

  1. Can you explain the changes made to "db/mobile.php"?

The changes we made in this file is we called the PHP method mobile_questionnaire_init (point 2).

  1. It appears you have replaced the "styles_app.css" with the new "mobile_css.php" file. Given that, should the "styles_app.css" be deleted?

Thanks for correct me. I added some code into styles_app.css and removed mobile.css.

  1. Can the "questionnaire_prep_answer_sort_question" function in locallib be refactored into one of the classes? Maybe the specific response class?

Thanks for your suggestion. I moved all sort methods from locallib to responsetype\sorting.

  1. Can you add the new question type to the unit test for csv exporting?

Of course. I added some UTs.

Could you please review the code again?

Many thanks

@rezeau
Copy link
Contributor

rezeau commented Aug 2, 2023

Cannot test and review because this PR must be re-based against current MOODLE_400_STABLE first.

mchurchward and others added 7 commits August 11, 2023 11:46
@mchurchward
Setting up release for Moodle 4.1 onward.
686880b
@lucaboesch @FelixDiLenarda @mchurchward
Allow localized answer options to be displayed correctly in condition…
5fc9623 
…s. (PoetOS#449)

* Resized the monologo.svg and png to 24x24 px (PoetOS#477)

* Fixing issues from plugins database checks (PoetOS#483)

* Removing illegal console commands. Script may not be needed.

* Fixing illegal line length.

* Fixing docblocks and unused variables.

* Fixing css spaces.

* Adding a true return.

* Fixing JSON context.

* Allow localized answer options to be displayed correctly in conditions.

---------

Co-authored-by: FelixDiLenarda <[email protected]>
Co-authored-by: Mike Churchward <[email protected]>
@toanlamt
Questionnaire: improved accessibility for essay box type (PoetOS#506)
133f4c7 
* The text editor should be programmatically associated with the question

Co-authored-by: toanlam <[email protected]>
@mchurchward
Removing 4.3 for now.
64457dd
@tailetan @emanoylov
Questionnaire\Accessibility\Slider: The slider values should be assoc…
f55147e 
…iated with the labels and the slider should be programmatically associated with the question (PoetOS#495)

* Questionnaire: More meaningful error than nopermissions (PoetOS#488)

Co-authored-by: Emanoil Manoylov <[email protected]>

* Questionnaire\Accessibility\Slider: The slider values should be associated with the labels #674064

---------

Co-authored-by: emanoylov <[email protected]>
Co-authored-by: Emanoil Manoylov <[email protected]>
Co-authored-by: tai.letan <[email protected]>
@lamtranb @emanoylov
Questionnaire\Accessibility\Rate: table does not have a programmatica…
d27f42b 
…lly associated caption (PoetOS#497)

* Questionnaire: More meaningful error than nopermissions (PoetOS#488)

Co-authored-by: Emanoil Manoylov <[email protected]>

* Questionnaire\Accessibility\Rate: table does not have a programmatically associated caption

---------

Co-authored-by: emanoylov <[email protected]>
Co-authored-by: Emanoil Manoylov <[email protected]>
Co-authored-by: lamtranb <[email protected]>
@tailetan
Questionnaire\Accessibility\Numeric: Instructions not programmaticall…
cb07cc2 
…y associated with field #672028 (PoetOS#496)

Co-authored-by: tai.letan <[email protected]>
@tailetan
Copy link
Contributor

I have already rebased this with MOODLE_401_STABLE.
FYI: @rezeau, @mchurchward

@mchurchward
Copy link
Contributor

For whatever reason, there are still merge conflicts that need to be resolved. This is usually due to needing rebasing.

lamtranb and others added 3 commits September 26, 2023 11:14
@lamtranb
Questionnaire\Accessibility\Rate: The form controls within the table …
3813cba 
…are not accessible (PoetOS#505)

Co-authored-by: lamtranb <[email protected]>
@emanoylov
Questionnaire/Accessibility: Check boxes missing group label (PoetOS#501
829c180 
)

Co-authored-by: Emanoil Manoylov <[email protected]>
@opitz
Fixing issues PoetOS#502 and PoetOS#507 (PoetOS#508)
04023c2 
* now using core_external\external_api - fixes PoetOS#502

* using valid $required parameter VALUE_REQUIRED - fixes PoetOS#507

---------

Co-authored-by: Matthias Opitz <[email protected]>
@tailetan
Copy link
Contributor

tailetan commented Oct 5, 2023

I have already rebased this with MOODLE_401_STABLE.
FYI: @mchurchward

@lamtranb
Copy link
Contributor Author

Dear @tailetan , this branch has conflicts again. Could you please resolve it?

@tailetan
Copy link
Contributor

@lamtranb, I've just rebased the latest code. Thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mchurchward mchurchward mchurchward left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@lamtranb @rezeau @tailetan @mchurchward @lucaboesch @toanlamt @emanoylov @opitz