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

SAK-50645 Lessons misplaced items when added to the page #12990

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

hornersa
Copy link
Contributor

Jira: https://sakaiproject.atlassian.net/browse/SAK-50645

Refer to videos attached to the jira which demo the bug and the proposed fix.

@@ -3290,6 +3290,7 @@ function buttonOpenDropdowna() {
addAboveItem = addAboveLI.find("span.itemid").text();
$(".addbreak").show();
openDropdown($("#addContentDiv"), $("#dropdownc"), msg('simplepage.add-above'));
$("#addContentDiv button.btn-close").click(closeDropdown);
Copy link
Contributor

Choose a reason for hiding this comment

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

what exactly is this doing? my read is that you are attaching a new click handler to this button.btn-close so that the next time it is clicked, it will call the closeDropdown function. is that your intent?

Copy link
Contributor Author

@hornersa hornersa Oct 28, 2024

Choose a reason for hiding this comment

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

Yes, we need the modal's close button when clicked to clean up (reinitialize) some var settings occurring when the Add Content 'above this item' modal or the Add Content 'at the bottom of this section' modal is opened.

Fwiw, the rationale for the function name "closeDropdown" is borrowed from show-page.js naming used in Sakai 21/22 for similar intents.

I'm open to suggestions. How would you like to proceed?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, we need the modal's close button when clicked ...

What about when it's not clicked, and the user instead uses escape to close it out?

Fwiw, the rationale for the function name "closeDropdown" is borrowed from show-page.js naming used in Sakai 21/22 for similar intents.

I would recommend just using clear method naming instead of trying to re-use old naming.

I'm open to suggestions. How would you like to proceed?

I don't know this code. But can you cleanup somewhere else? Can you just clean these vars in openDropdown instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Trying to capture the various other closing events (something I obviously overlooked in hindsight) was more complicated than readding an explicit 'open' function for the main 'Add Content' button. Thus, I've since proceeded with the latter approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since porting this new commit to my local instance of 23.x and testing there, I'm finding some problems. I'll revert this to draft until I can resolve those issues.

Copy link
Contributor Author

@hornersa hornersa Oct 28, 2024

Choose a reason for hiding this comment

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

Seems that I was missing id="addcontent" from the relevant button in ShowPage.html.

(What has me still somewhat scratching my head though is that I thought I had seen that id attribute for this button present in the DOM without my explicitly adding it. Now however I can't locate where it might be sourced... whether via ShowPage.html, show-page.js, or ShowPageProducer.java.)

@hornersa hornersa force-pushed the SAK-50645 branch 2 times, most recently from e211485 to 7bbfc33 Compare October 28, 2024 21:41
@hornersa hornersa marked this pull request as draft October 28, 2024 22:35
@hornersa hornersa marked this pull request as ready for review October 28, 2024 23:12
@ern ern changed the title SAK-50645 Lessons: Misplaced items when added to the page SAK-50645 Lessons misplaced items when added to the page Oct 29, 2024
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