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

Compatibility v10 #56

Merged
merged 19 commits into from Sep 5, 2022
Merged

Conversation

adrien-schiehle
Copy link
Contributor

The first three commits seems to come from somethink we already merged : Refresh button + category sort
I don't know why it is back in this PR :(

The rest is V10 compliance + entry pages for new Journal management

13-08-2022:
I recently added some additionnal changes du to another change in WA API.
And while doing it, I added a new buttion for shrinking categories in the journal.
(When you have a big WA world, it become difficult to navigate without it)

@adrien-schiehle
Copy link
Contributor Author

Conflict seems to come from the fact I didn't retrieve all the changes that has been done during reviews.

I will try to see today how to do it with githubDesktop

Improve image identification and category sorting
# Conflicts:
#	module/framework.js
#	module/journal.js
@adrien-schiehle
Copy link
Contributor Author

adrien-schiehle commented Aug 13, 2022

We may need to discuss what we want in term of pages for this v10

For now, I have :

  • One main page called Article / Character with the main WA Article content
  • Some additional pages named Portrait / Cover / Image. (It's what was previously in journal.img)
  • Two pages named Side content and Secrets for other sections. (Only section with id seeded is for now inside Secrets page)
  • One last one for Relations. (this one is not totally ok for now)

There will also be a question on which one should be displayed first : Article or Image

Copy link
Contributor

@aaclayton aaclayton left a comment

Choose a reason for hiding this comment

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

I like what you've done here to allocate the sections of the source WA article to different pages. I am not really sold on the idea of having a bunch of settings, do you think that users will really care to configure this? Could we simplify things and make this the default behavior?

module.json Outdated Show resolved Hide resolved
@@ -106,5 +106,61 @@ export default class WorldAnvilConfig extends FormApplication {
config: true
});

// Add the customizable labels for each importable page
//-------------------
game.settings.register("world-anvil", "mainArticlePage", {
Copy link
Contributor

Choose a reason for hiding this comment

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

These settings feel like a level of customization that may not be necessary to me - I wonder if we could keep it simpler at first and see what feedback we get from users?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See comment bellow.

The parameters are far from being mandatory. An good alternative could be to create translation file for every language. Or ate the least put how to create on with the necessary data inside the Readme file.

module/journal.js Outdated Show resolved Hide resolved
@@ -2,15 +2,19 @@
{{#if this.hasContent}}
<section class="category" data-category-id="{{ this.id }}" data-folder-id="{{ this.folder.id }}">
<header class="category-header flexrow">
<i title="{{ localize 'WA.ButtonUnshrinkCategory' }}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Places like this are a good opportunity to use the new tooltips API instead of a standard HTML title

Suggested change
<i title="{{ localize 'WA.ButtonUnshrinkCategory' }}"
<i data-tooltip="{{ localize 'WA.ButtonUnshrinkCategory' }}"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've put data-tooltip= where we had title=. I gives a nice display.

I'm curious: Is there a way to alter those tooltip css ?
I will surely use it in my custom system. But all my custom made tooltips are written in black, with a cornsilk background

Comment on lines +47 to +49
#world-anvil-browser .window-content {
max-height: 80vh;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without it, if you have a give world anvil world, it tends to take the whole screen.
I found it doesn't fit well, so added this.

What it does without this parameter :
image

@@ -114,6 +114,9 @@ Hooks.on("renderJournalSheet", (app, html, data) => {
}
}
}
});

Hooks.on("renderJournalPageSheet", (app, html, data) => {

// Activate cross-link listeners
html.find(".wa-link").click(event => {
Copy link
Contributor

Choose a reason for hiding this comment

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

It does not need to be part of this merge, but we have a new V10 feature for declaring custom text enrichments which can be used instead of this custom logic.

You can add a TextEditorEnricher to CONFIG.TextEditor.enrichers which provides a regex pattern and a replacer callback function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hoho,
Good to know. I will try to se how it works

@adrien-schiehle
Copy link
Contributor Author

I like what you've done here to allocate the sections of the source WA article to different pages. I am not really sold on the idea of having a bunch of settings, do you think that users will really care to configure this? Could we simplify things and make this the default behavior?

I don't really remember why I choose to put them as parameters.

  • Perhaps, to allow people who only import characters to hand the page named Character instead of Article ?
  • Or Perhaps to allow peaple using another language to use the module "as it is" without having to get a whole module translation file. (The GM seeing english text inside WAJournalBrowser is ok. But players seeing text in english when opening a Journal can tend to be a little anticlimatic)

I'm french myself.
Until now, I didn't see the need to add a fr.json file to the project, since it was mainly for GM.
I'm lucky : Article, Portrait, and Secrets are spelled the same in french, so I won't have the need. But it will be different for german people

@aaclayton
Copy link
Contributor

Thanks! I'll merge this in and cut a V10 compatible release :)

@aaclayton aaclayton merged commit ba09fec into foundryvtt:master Sep 5, 2022
@adrien-schiehle adrien-schiehle deleted the compatibilityV10 branch September 10, 2023 21:48
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.

None yet

2 participants