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

cc: restructure edit page JS for improved maintainability #1646

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

Conversation

jo3-l
Copy link
Contributor

@jo3-l jo3-l commented May 12, 2024

Refactor the JS script for the custom command edit page to be more maintainable and extensible.

As noted previously in #1619 (comment), the current JS script for the custom command edit page has grown rather complex and difficult to understand over time. Particularly egregious is the current logic for showing/hiding various parts of the page depending on the trigger type:

function triggerTypeChanged() {
var dropdown = $("#trigger-type-dropdown")
if (dropdown.val() === "interval_hours" || dropdown.val() === "interval_minutes") {
// Interval triggers
$("#interval-cc-run-now").removeClass("hidden")
$("#cc-time-trigger-details").removeClass("hidden");
$("#cc-text-trigger-details").addClass("hidden");
$("#cc-extra-settings").addClass("hidden");
$("#cc-reaction-trigger-details").addClass("hidden")
$("#trigger-warning").attr("hidden", true);
handleTimeTriggerChannelChange();
$("#require-no-channels-warning").addClass("hidden");
$("#require-no-roles-warning").addClass("hidden");
$("#time-trigger-no-channel-warning").removeClass("hidden");
} else if (dropdown.val() === "reaction") {
// Reaction triggers
$("#cc-reaction-trigger-details").removeClass("hidden")
$("#cc-extra-settings").removeClass("hidden");
$("#cc-time-trigger-details").addClass("hidden");
$("#cc-text-trigger-details").addClass("hidden");
$("#interval-cc-run-now").addClass("hidden")
$("#trigger-warning").attr("hidden", true);
$("#require-no-channels-warning").removeClass("hidden");
$("#require-no-roles-warning").removeClass("hidden");
$("#time-trigger-no-channel-warning").addClass("hidden");
} else if (isTextTrigger(dropdown.val())) {
// Other message triggers
$("#cc-text-trigger-details").removeClass("hidden");
$("#cc-extra-settings").removeClass("hidden");
$("#cc-reaction-trigger-details").addClass("hidden")
$("#cc-time-trigger-details").addClass("hidden");
$("#interval-cc-run-now").addClass("hidden")
$("#trigger-warning").attr("hidden", true);
$("#require-no-channels-warning").removeClass("hidden");
$("#require-no-roles-warning").removeClass("hidden");
// $("#trigger-warning").text("No trigger set, this command will only be able to be called from other commands")
$("#time-trigger-no-channel-warning").addClass("hidden");
} else {
// No automatic trigger
$("#cc-text-trigger-details").addClass("hidden");
$("#cc-extra-settings").addClass("hidden");
$("#cc-reaction-trigger-details").addClass("hidden")
$("#cc-time-trigger-details").addClass("hidden");
$("#interval-cc-run-now").addClass("hidden")
$("#trigger-warning").removeAttr("hidden");
$("#trigger-warning").text("No trigger set, this command will only be able to be called from other commands")
$("#require-no-channels-warning").addClass("hidden");
$("#require-no-roles-warning").addClass("hidden");
$("#time-trigger-no-channel-warning").addClass("hidden");
};
if (dropdown.val() === "cmd") $("#command-trigger-prepended-prefix").show();
else $("#command-trigger-prepended-prefix").hide();
$("#trigger-help").children().each(function (i, v) {
$(v).attr("hidden", true);
});
$("#trigger-desc-" + dropdown.val()).removeAttr("hidden");
}

Though avoiding overabstraction is important, the sheer repetitiveness and density of this code (~80 lines with very minor differences between if branches, obscuring the function of the code) renders it extremely difficult to understand and maintain. The rest of the script is not much better in this regard.


This PR therefore attempts to refactor the entire script while not changing functionality. The best solution would be to use a frontend framework that allows for proper state management, à la React, but such a broad change is unfeasible at the moment.

The changes introduced are best reviewed by opening the new file separately as opposed to examining the diff.

General outline of changes

Broadly speaking, the responsibilities of the current JS script are as follows:

  1. Support the shortcut Alt + Shift + S to save the custom command.
  2. Show/hide certain parts of the edit interface depending on the trigger type. For instance, we want to show the Require roles menu for a command-type trigger, but not for an interval-type trigger.
  3. Show/hide warnings for common configuration mistakes as appropriate. For instance, selecting no channel for an interval command is indicative of user error, as the command will never run.
  4. Maintain character counters for the custom command response box and the custom command name.

In this refactor, we concern ourselves mainly with improving the implementation of 2) and 3), which were previously rather ad-hoc (see the quoted segment of code above for 2), for instance.)

  • For 2), we create a list of elements in the page that should be shown for a given trigger type; when needed, some glue code runs through the list, hiding elements for other trigger types and showing elements for the current trigger type only. If we ever wish to add a new component to the page that should be shown only for a certain trigger type, then, it is as easy as to add the corresponding element ID to the list.
  • For 3), we establish a common interface for checks (which generate warnings) and move existing code into this interface. The main complexity here is that some warnings are applicable only to certain trigger types: for instance, interval commands have no concept of required roles so an empty list of required roles should not display a warning. So each check defines a set of applicable trigger types, and some glue code runs through all checks and hides warnings when needed, similar to the implementation for 2).
  • The implementations for 1) and 4) are left largely untouched.

Though it may seem as though we have introduced some abstraction in this process, the end result is not only more modular but also drastically shorter than the previously implementation by about 40 lines overall.

To ensure minimal behavior change, I have visually compared the result on my selfhost with the corresponding page on YAGPDB and have confirmed, to the best of my ability, that there are no problematic differences. But this claim should of course be verified and tested more thoroughly before release.

@jo3-l jo3-l changed the base branch from master to dev May 12, 2024 01:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant