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

Moodle 4.5 branch #133

Merged
merged 3 commits into from
Dec 13, 2024
Merged

Moodle 4.5 branch #133

merged 3 commits into from
Dec 13, 2024

Conversation

Syxton
Copy link
Owner

@Syxton Syxton commented Dec 10, 2024

No description provided.

@PhMemmel
Copy link
Collaborator

@Syxton Thanks for the work! I had a quick look.

Your patch fixes the basic issue, great!

However, I would suggest to make the three dropdowns resemble the current course structure by moving the subsections to the "correct" place and also add a small indentation or a small symbol as prefix to the name to them. What do you think?

@Syxton Syxton force-pushed the MOODLE_405_DEV branch 3 times, most recently from 99bfeff to 0348a49 Compare December 12, 2024 14:49
@Syxton
Copy link
Owner Author

Syxton commented Dec 13, 2024

@PhMemmel ok, I think I have a better solution now. Take a look when you have time.

Copy link
Collaborator

@PhMemmel PhMemmel left a comment

Choose a reason for hiding this comment

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

This looks like a very good implementation, great!

Just 2 things:

  • Updating a section name for example will fill up the section list dropdown with duplicate entries. I suggest to add sections = []; before line 97
  • I'm not sure what the users would expect: By selecting a section only the course modules in a section will be selected, but not the course modules inside the subsections. Is this intended? Or would a user expect to select all course modules including the course modules in the subsections of the section he selected?

amd/src/checkboxmanager.js Outdated Show resolved Hide resolved
amd/src/checkboxmanager.js Outdated Show resolved Hide resolved
amd/src/checkboxmanager.js Outdated Show resolved Hide resolved
amd/src/checkboxmanager.js Show resolved Hide resolved
amd/src/checkboxmanager.js Outdated Show resolved Hide resolved
@Syxton
Copy link
Owner Author

Syxton commented Dec 13, 2024

@PhMemmel Thank you for looking through it. I have made the requested changes. I think we could look into creating those fancy new menus like Moodle uses for groups and availability that could allow selecting each section individually but also have a button for selecting all subsections as well. For now we'll keep it as is because we can select by section but can't subtract selections by section. It would be a bit faster to select everything in a section and its subsections, but would be very slow to select all in a section and its subsections then deselect everything in each subsection module by module if you only wanted the outer sections modules.

@PhMemmel
Copy link
Collaborator

Alright.

I just discovered one maybe minor issue:

  • Enable moodle mass actions
  • Click on the checkbox of a section
  • You then will not be able to select a course module inside this section
  • However, you will still be able to select course modules inside this section by using our "select" dropdown.

Do you consider this as a problem? I mean... they are still selected, the checkboxes are just greyed out, but the checks are there, and applying an action also works :) We just break how the moodle mass actions work.

@Syxton
Copy link
Owner Author

Syxton commented Dec 13, 2024

@PhMemmel Hmm. While it isn't necessarily ideal with how it looks, Seems to me that this functionality might offer a more expected behavior than if block mass action selection being ignored if a section was selected by accident. However, we could clear any section selection upon using mass action selection so that they don't overlap, much like Moodle clears the checkboxes of the modules when the section is selected. What do you think?

@PhMemmel
Copy link
Collaborator

I'm fine with how it currently behaves. So let's go with it. Testing is fine, CR too, so feel free to merge if you do not have any other issues with moodle 4.5 compatibility you want/need to address.

@Syxton
Copy link
Owner Author

Syxton commented Dec 13, 2024

@PhMemmel Agreed, it is a little more complicated than I thought anyway. If you select an individual activity, Moodle disables the section checkbox, but if we select the activities using mass actions block, we don't disable the section check boxes. seems better to just let there be a visual discrepancy but allow both tools to just work.

@Syxton Syxton merged commit a412d56 into master Dec 13, 2024
8 checks passed
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.

2 participants