Skip to content

🧹 Refactor RaindropFetchModal onOpen to use helper methods #52

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

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

🧹 Refactor RaindropFetchModal onOpen to use helper methods #52

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 17 additions & 6 deletions src/modals.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,14 +30,24 @@ export class RaindropFetchModal extends Modal {
this.vaultPath = this.plugin.settings.defaultFolder;
}


onOpen() {
const { contentEl } = this;
contentEl.empty();
contentEl.addClass('make-it-rain-modal'); // For potential future styling

new Setting(contentEl).setName('Fetch options').setHeading();

// --- Fetch criteria Section ---
this.buildFetchCriteriaSection(contentEl);
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot Apr 26, 2026

Choose a reason for hiding this comment

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

medium

While extracting the onOpen logic into helper methods is a significant improvement, buildFetchCriteriaSection remains quite large and handles multiple responsibilities (e.g., vault location, collection fetching, tag filtering). Consider further decomposing it—for example, by extracting the collection list fetching and rendering logic (lines 81-142) into its own private method—to enhance readability and maintainability.

this.buildNoteOptionsSection(contentEl);
this.buildTemplateOptionsSection(contentEl);

contentEl.createEl('hr');

this.buildActionButtons(contentEl);
}

private buildFetchCriteriaSection(contentEl: HTMLElement) {
new Setting(contentEl).setName('Fetch criteria').setHeading();

new Setting(contentEl)
Expand Down Expand Up @@ -188,8 +198,9 @@ export class RaindropFetchModal extends Modal {
this.filterType = value as RaindropType | 'all';
});
});
}

// --- Note Options Section ---
private buildNoteOptionsSection(contentEl: HTMLElement) {
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot Apr 26, 2026

Choose a reason for hiding this comment

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

medium

The synchronization logic between the 'Fetch only new items' and 'Update existing notes' toggles (found in the body of this method) currently relies on DOM queries and CSS classes. This approach is brittle and less type-safe. A more robust pattern would be to store references to the ToggleComponent instances and use their setValue methods to keep the UI in sync.

new Setting(contentEl).setName('Note options').setHeading();

const appendTagsSetting = new Setting(contentEl)
Expand Down Expand Up @@ -276,8 +287,9 @@ export class RaindropFetchModal extends Modal {
}
});
});
}

// --- Template Options Section ---
private buildTemplateOptionsSection(contentEl: HTMLElement) {
if (this.plugin.settings.isTemplateSystemEnabled) {
new Setting(contentEl).setName('Template overrides (optional)').setHeading();

Expand Down Expand Up @@ -316,10 +328,9 @@ export class RaindropFetchModal extends Modal {
cls: 'setting-item-description'
});
}
}

contentEl.createEl('hr');

// --- Action Buttons ---
private buildActionButtons(contentEl: HTMLElement) {
const buttonsEl = contentEl.createDiv({ cls: 'modal-button-container' });
new ButtonComponent(buttonsEl)
.setButtonText('Fetch raindrops')
Expand Down
Loading