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

New attempt at restoring the radar chart_type to questionnaire feedback #490

Conversation

rezeau
Copy link
Contributor

@rezeau rezeau commented Jun 23, 2023

This is my 3rd attempt at restoring this really needed chart type. Don't know why I closed my similar PR. I do hope this new PR can be merged any time soon. It's only a few lines changed in the code; it used to work when I first created it years ago and it disappeared from the code some time ago, no idea why.
Thanks in advance,
Joseph

FelixDiLenarda and others added 3 commits May 16, 2023 14:13
* 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.
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 Joseph... I want to make sure this is correct.
There seems to be three sets of logic. One for 'global', one for two feedback sections, and one for more than two feedback sections? And it appears that the graphs offered in the form are different depending on these sections?

Can you describe what the logic does, and the rules that impact them in the PR comments please? Maybe add some comments to the code too.

@rezeau
Copy link
Contributor Author

rezeau commented Jul 5, 2023

@mchurchward I'm looking into this, will come back in a couple of days.

Signed-off-by: Joseph Rézeau <[email protected]>
@rezeau
Copy link
Contributor Author

rezeau commented Jul 6, 2023

@mchurchward
Here's the work I've done on my new commit [Revision 01 of PR #490]

feedback_form.php
Added a number of comments.
Reduced the display of the Chart type dropdown lists from 3 to 1 (or zero).
Logic table:
Feedback options | Chart type dropdownn list
No Feedback messages | Not displayed
Global Feedback | None / Bipolar bars / Vertical Progress bar
Feedback Sections 1 to 2 | None / Bipolar bars / Horizontal bars
Feedback sections: at least 3 | None / Bipolar bars / Horizontal bars / Rose / Radar

Added a check on the number of questions available, to prevent the erroneous use of the Feedback options -> Feedback sections in case of only one question available in the Questionnaire (which should normally never happen, but...)

Currently on the Feedback options page there are 2 buttons:
Save settings & Save settings and edit Feedback Sections. There is no way to quit that page other than clicking on one of the Advanced settings, Questions, Preview, etc. tabs above.
Changed the Save settings button to Save and display, which saves the current Feedback options and exits the page and goes to the main Questionnaire->View page.

Currently on the Feedback sections page there are 2 buttons:
Save changes, Delete this section & Cancel.
There is no way to quit that page other than clicking on one of the Advanced settings, Questions, Preview, etc. tabs above.
New set of buttons as follows:
Delete this section: does what it says
Save changes: saves current changes
Feedback options: saves current changes and goes (back) to the Feedback options page in the case that the number of sections added to the form will now enable more chart types to be available. The Documentation will have to explain that, of course.
Save and display: saves the current changes and and goes to the main Questionnaire->View page.
Removed the Cancel button which is no longer needed I think.

@rezeau
Copy link
Contributor Author

rezeau commented Jul 19, 2023

Hi @mchurchward
I would really like this PR to be merged ASAP, as I want to add a new option to the Personality test mode. How can I get someone to review my PR so it can be merged?
Thanks

PS. I see "At least 1 approving review is required by reviewers with write access". Who are those "reviewers with write access"?

@mchurchward mchurchward changed the base branch from MOODLE_400_STABLE to MOODLE_401_STABLE September 26, 2023 15:17
@mchurchward
Copy link
Contributor

Joseph. has this PR been superceded now?

@rezeau
Copy link
Contributor Author

rezeau commented Oct 3, 2023

@mchurchward Yes, it's been replaced by #510

@rezeau
Copy link
Contributor Author

rezeau commented Oct 4, 2023

superseded by #510

@rezeau rezeau closed this Oct 4, 2023
@rezeau rezeau deleted the restore_radar_charttype_june_2023 branch October 4, 2023 08:54
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