Conversation
…ForgeVTT/forge-compendium-browser
zeel01
left a comment
zeel01
left a comment
There was a problem hiding this comment.
Looks good for the most part, but I think we need to clean up the console output. If something is really important for debugging we should prefer console.debug() as the verbose log level if nothing is going wrong. I was going to suggest we could use the log levels from https://foundryvtt.com/packages/_dev-mode but it seems to not have been updated past v9...
src/apps/compendium-browser-app.js
Outdated
| async _prepareContext(options) { | ||
| console.warn("CompendiumBrowserApp._prepareContext", options); | ||
| const context = await super._prepareContext(options); | ||
| return context; | ||
| } | ||
|
|
||
| _onRender(context, options) { | ||
| console.warn("CompendiumBrowserApp._onRender", context, options); |
There was a problem hiding this comment.
Should these console warnings remain in the code?
src/apps/compendium-browser-app.js
Outdated
| export class CompendiumBrowserApp extends CompendiumBrowserAppBase { | ||
| constructor(book, options = {}) { | ||
| super(null, options); | ||
| console.warn("CompendiumBrowserApp.constructor", book, options); |
src/forge-compendium-browser.js
Outdated
| } | ||
|
|
||
| static async onMessage(data) { | ||
| console.warn("ForgeCompendiumBrowser.openBrowser", data); |
src/forge-compendium-browser.js
Outdated
| } | ||
|
|
||
| static openBrowser(book) { | ||
| console.warn("ForgeCompendiumBrowser.openBrowser", book); |
Eranziel
left a comment
Eranziel
left a comment
There was a problem hiding this comment.
A good start! Still needs some work, could be done in a follow-up PR.
- Light mode CSS is borked.
- Actor sheets don't render in a usable way.
- Item sheets don't render in a usable way.
- Journals don't render. (Vanilla journal show behind compendium browser window.)
| "compatibility": { | ||
| "minimum": "9", | ||
| "verified": "12" | ||
| "verified": "13" |
There was a problem hiding this comment.
issue: Given that we're moving to ApplicationV2, this will not work on older than v12. minimum should be updated to 12, and we'll need to test with v12 to ensure everything does work correctly there.
Also, since this next version will drop support for v9 - v11, we could update the manifest to drop the legacy fields (minimumCoreVersion, compatibleCoreVersion, etc...)
src/apps/compendium-browser-app.js
Outdated
|
|
||
| export class CompendiumBrowserApp extends Application { | ||
| let CompendiumBrowserAppBase; | ||
| if (foundry?.applications?.api?.ApplicationV2) { |
There was a problem hiding this comment.
I don't think it's worth putting ourselves through knots to maintain AppV1 compatibility here. I think we should just use AppV2 and set min compatibility to v12.
- Improved code organization - Made rendering compatible with v13 applications - Made rendering compatible with new 5e system
…ame" element instead.
|
|
@qalucaspacheco1 QA notes:
|
3 participants
mainby mistake when creating this branch. The only thing I managed to revert was the git history somehow, but the changes are still on there.