Third step secrets management - [merged] #48
Comments
|
In GitLab by @adrien.schiehle on Nov 19, 2021, 09:56 added 1 commit
|
|
changed target branch from |
|
It would be nice to use .svg icons instead of .png for smaller filesize. We can also consider using font awesome icons very easily which are already included in Foundry VTT. |
|
In GitLab by @adrien.schiehle on Nov 19, 2021, 13:56 added 4 commits
|
|
Document as |
|
It might be more direct to store a module-level cached reference to the |
|
I think I probably need to chat with you some to understand the high-level context of the change. I'm not very familiar with secrets on World Anvil so I want to understand them a little better before reviewing the code to implement them on the Foundry side. |
|
In GitLab by @adrien.schiehle on Nov 19, 2021, 14:09 I'm not using the full extend of what can be used inside World-Anvil. It has a secret section which visibility for only some subscriber groups. Since (for now), I don't intend to correlate foundry players and logins inside world-anvil, I've chosen to not use them inside the import. (And in fact, they are not available is Dimitri's current api) I'm only using this (available for each article): I chose to split its content in multiple secrets, by checking the use of |
|
In GitLab by @adrien.schiehle on Nov 19, 2021, 14:14 Commented on module/journal.js line 51 Something like that ? Hooks.once('ready', () => {
game.worldAnvilBrowser = new WorldAnvilBrowser();
}); |
|
In GitLab by @adrien.schiehle on Nov 19, 2021, 15:32 Commented on module/journal.js line 51 changed this line in version 5 of the diff |
|
In GitLab by @adrien.schiehle on Nov 22, 2021, 11:31 I'm using a background image with a left padding. And I use different icons (and colors) depending on the secret state : .wa-section.wa-secret {
//...
padding: 0 5px 0 30px;
background-image: url('icons/hidden_secret.png');
background-repeat: no-repeat;
background-position-x: left;
background-position-y: center;
background-size: 28px;
}
.wa-section.wa-secret.hidden {
background-image: url('icons/hidden_secret.png');
}
.wa-section.wa-secret.hidden:hover {
background-image: url('icons/hidden_secret_hover.png');
}Can svg icons have colors ? I can also try to switch to fontawesome icons, but it would means adding text instead of just using a background image. I can try if you want. But I'm not convinced. |
|
In GitLab by @adrien.schiehle on Nov 23, 2021, 20:06 added 1 commit
|
|
In GitLab by @adrien.schiehle on Nov 23, 2021, 20:07 The custom secret management has been transfered here : https://gitlab.com/adrien.schiehle/world-anvil-secrets |
|
In GitLab by @adrien.schiehle on Nov 23, 2021, 20:09 added 1 commit
|
|
In GitLab by @adrien.schiehle on Nov 23, 2021, 20:13 added 1 commit
|
|
In GitLab by @adrien.schiehle on Nov 25, 2021, 21:06 added 1 commit
|
|
I don't think we should specifically pre-suppose that the user of this hook will only care about flags. They may want to do other things as well. This hook should more generally handle pre-processing of content before the entry is updated. I would prefer something like: Hooks.callAll(`WAUpdateJournalEntry`, entry, content); |
|
We should have a corresponding hook for cases where we are first creating a journal entry: Hooks.callAll(`WACreateJournalEntry`, createData, content); |
|
I don't think this is the right hook design. I feel that we should do our default parsing first and then pass the parsed result to a hook to modify/change if they want. Doing hooks first and then merging it is not the right design pattern. Dealing with |
|
This is where we should be calling the hook const parsedData = {
html.
img: image,
waFlags: waFlags
}
Hooks.callAll("WAParseArticle", article, parsedData); |
|
This section needs a line of code commentary to make it clear what/why we are doing this. |
|
Code style, please use parentheses to enclose composite logical conditions: game.folders.find(f => (f.data.type === "JournalEntry") && (f.getFlag("world-anvil", "categoryId") === category.id)); |
|
I'm not sold on displaying in the UI whether an article has secrets or not. We don't do that in the main Journal Entry UI so I don't know why it's needed here either. |
|
I don't really like using css |
|
Overall this looks better, but I think there is still a little bit too much complexity due to your handling of hooks. Please see my comments and let's discuss in Discord if you have any questions/feedback. Thanks! |
|
requested review from @aaclayton |
|
In GitLab by @adrien.schiehle on Nov 26, 2021, 11:13 Commented on templates/journal.hbs line 62 Well, I'm in fact using it as a teaser for the world-anvil-secrets module 👼 You're right saying it has no meaning in the core module. When hovering it, it says :
If you think it's uncalled for, I will remove it. |
|
In GitLab by @adrien.schiehle on Nov 26, 2021, 11:17 Commented on module/framework.js line 116 I agree on this. I was too much focused on my current needs and didn't do something which could be used in other circumstances. I will work on that |
|
In GitLab by @adrien.schiehle on Nov 26, 2021, 11:27 Commented on module/framework.js line 161 I didn't want to have to parse the html content to remove already added data. It seems far less complicated at first but it may fall in the same problem as your previous comments. If I want to keep to totally generic, I will need to do as you say. |
|
In GitLab by @adrien.schiehle on Nov 26, 2021, 13:13 Commented on module/framework.js line 116 changed this line in version 11 of the diff |
|
In GitLab by @adrien.schiehle on Nov 26, 2021, 13:13 Commented on module/framework.js line 161 changed this line in version 11 of the diff |
|
In GitLab by @adrien.schiehle on Nov 26, 2021, 13:13 Commented on module/framework.js line 231 changed this line in version 11 of the diff |
|
In GitLab by @adrien.schiehle on Nov 26, 2021, 13:13 Commented on module/framework.js line 223 changed this line in version 11 of the diff |
|
In GitLab by @adrien.schiehle on Nov 26, 2021, 13:13 added 1 commit
|
|
In GitLab by @adrien.schiehle on Nov 26, 2021, 13:19 Commented on module/framework.js line 439 changed this line in version 12 of the diff |
|
In GitLab by @adrien.schiehle on Nov 26, 2021, 13:19 added 1 commit
|
|
In GitLab by @adrien.schiehle on Nov 26, 2021, 13:20 Put in a separated module. Not in this MR anymore. |
|
Looks good! I am going to merge this into the secrets branch and then test it locally and make some small changes as I do that testing. Thanks @adrien.schiehle |
|
approved this merge request |
In GitLab by @adrien.schiehle on Nov 17, 2021, 10:29
Merges third_step_secrets_management -> secrets
Next step : Secret management.
Only Storyteller seeds are managed for now. I've chosen to split those seeds by checking if there is some
<h3 ...>inside of it. Each h3 indicating the start of a new secret. It will allow GMs to reveal secrets step by step for a given article.<h3>inside what we retrieve are what we write as[h2]inside BBCode.It works like that :
The text was updated successfully, but these errors were encountered: