-
Notifications
You must be signed in to change notification settings - Fork 40
Allow batch / folder import by id as well as title #207
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
Allow batch / folder import by id as well as title #207
Conversation
| contentEl.createEl('h3', { text: "Name of 'id' metadata field to use in API query (if present, will be used instead of title)." }); | ||
|
|
||
| const idFieldNameComponent = new TextComponent(contentEl); | ||
| idFieldNameComponent.inputEl.style.width = '100%'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would normally say: "Styles should be added with CSS classes." But the code above also doesn't use CSS classes, so this is fine.
|
This PR is a nice idea, but I think it should handle the case where a user wants to only import by one, Using smaller text below the heading to explain what the difference between |
…id lookup methods
src/main.ts
Outdated
| erroredFiles.push({ filePath: file.path, error: `no search results` }); | ||
| continue; | ||
| } | ||
| } else if (lookupMethod == 'id') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prefer an enum over a hard-coded string for lookupMethod
export enum BulkImportLookupMethod {
ID = 'id',
TITLE = 'title',
}| }); | ||
| } | ||
|
|
||
| createDropdownEl(parentEl: HTMLElement, label: string, onChange: { (value: string): void }, options: { value: string; display: string }[]): void { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prefer the type onChange: (value: string) => void
| { value: 'title', display: 'Title' }, | ||
| { value: 'id', display: 'ID' }, | ||
| ]; | ||
| this.createDropdownEl(contentEl, 'Lookup media by', onlookupMethodChange, lookupMethodOptions); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can inline the two consts above
| for (const option of options) { | ||
| dropDownComponent.addOption(option.value, option.display); | ||
| } | ||
| wrapperEl.appendChild(dropDownComponent.selectEl); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this and some of the appendChild in the existing code are necessary, but I am not 100% sure on that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I double checked the final HTML and I think with the current setup they are required because the wrappers are styled to use flex to get the labels on the left and inputs on the right.
<div class="media-db-plugin-list-wrapper">
<div class="media-db-plugin-list-text-wrapper">
<span class="media-db-plugin-list-text">Lookup media by</span>
</div>
<select class="dropdown"
<option value="title">Title</option>
<option value="id">ID</option>
</select>
</div>.media-db-plugin-list-wrapper {
display: flex;
align-content: center;
margin-bottom: 5px;
margin-top: 5px;
}
.media-db-plugin-list-text-wrapper {
flex: 1;
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah ok.
|
I'm not sure if this is currently working, it doesn't give any errors but just silently fails and does nothing besides close the modal. |
Hmm, so no "MDB - bulk import error" file was generated? And there's nothing in the developer console either? Could you share which settings you tried (API, appending content or not, property name, any other related plugin settings) and some examples of the .md files with IDs in the fronttmatter that you were trying to batch import from? I haven't tested every permutaiton, but this works for me in a fresh vault with default settings (both |
|
Okay that helped me find the problem, my id field was not recognized as a string because it was just numbers (for MALAPI) and not enclosed with quotes, nothing had shown up in the console but I hadn't thought of checking MDB - bulk import error file which gave a clear reason why. Thanks for the help. |

This proposed change adds an option to select an 'id' field to use when batch importing rather than the default 'title' field. Currently, the 'id' field takes priority over 'title' if it's added.
This allows for a fully automatic batch import without having to display a modal to allow the user to select the correct media from a list, since an ID can only match at most a single result.
I tested this by batch importing ~300 games from my Steam library, exported via the Steam Web 'GetOwnedGames' API which provides Steam App IDs for an entire user library.
Not sure if this would be useful to anyone else, but thought I'd share just in case!