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
Cosmetics modifications - [merged] #43
Comments
Convention: localization keys are title case, for example |
For my own understanding of World Anvil use case, what's an example of why a user would want to disable this? Would it be ideal to toggle this at an overall world level (for all articles) or at an individual article level? |
I think the functionality to move the title out of the body to the journal entry itself is probably the right call. Do you think this even needs to be a setting? |
I think the change to move the "On WA" button to the header space is a good call. Again, do you think this needs to be a setting or is that simply a change we should make directly (without a setting)? |
this setting makes sense to me, but I think the setting name chosen here is a bit difficult to understand/unintuitive. Perhaps something like |
Nitpick, call this variable something like |
Prefer using an early continue to avoid an extra level of if ( !section.items ) continue; |
We don't need to pass both key and value when the variable name is the key name, so the prior syntax here is preferred. |
Thanks for your submission, this looks pretty good! I have a couple questions to get on the same page about. |
In GitLab by @adrien.schiehle on Sep 23, 2021, 01:59 Commented on lang/en.json line 7 changed this line in version 2 of the diff |
In GitLab by @adrien.schiehle on Sep 23, 2021, 01:59 added 1 commit
|
In GitLab by @adrien.schiehle on Sep 23, 2021, 03:24 Commented on module/config.js line 100 changed this line in version 3 of the diff |
In GitLab by @adrien.schiehle on Sep 23, 2021, 03:24 Commented on module/config.js line 128 changed this line in version 3 of the diff |
In GitLab by @adrien.schiehle on Sep 23, 2021, 03:24 Commented on module/framework.js line 69 changed this line in version 3 of the diff |
In GitLab by @adrien.schiehle on Sep 23, 2021, 03:24 Commented on wa.js line 92 changed this line in version 3 of the diff |
In GitLab by @adrien.schiehle on Sep 23, 2021, 03:24 added 1 commit
|
In GitLab by @adrien.schiehle on Sep 23, 2021, 03:30 Commented on lang/en.json line 7 Good to know. Fixed. |
In GitLab by @adrien.schiehle on Sep 23, 2021, 03:30 Commented on module/config.js line 100 When I first tried synchronize my WA articles, I found that sidebars display weren't great when synchronizing. (Can't be put on right side). In addition, they are integrated in the import even if you specify that you don't want to render then in WA.
But, after taking a step back, I may have been a little too violent. With a little more research, I just found that one of those article relations is named I can try to base my import on that instead.
|
In GitLab by @adrien.schiehle on Sep 23, 2021, 03:30 Commented on module/config.js line 100 I totally agree with that. I tried to do evolves that won't change current import for those who are ok with the current behavior. So I let an option for that. (Even if I don't see a case where it would be useful)
|
In GitLab by @adrien.schiehle on Sep 23, 2021, 03:30 Commented on module/config.js line 109 I agree too. Just did it in fear some people who already use the module won't like it. |
In GitLab by @adrien.schiehle on Sep 23, 2021, 03:31 Commented on module/config.js line 128 I'm ok with |
In GitLab by @adrien.schiehle on Sep 23, 2021, 03:31 Commented on wa.js line 92 I didn't know. Good to know. I still find it dangerous. If you rename you variable in your function, it directly makes the called method fail. But it's more concise, so let's do it. |
In GitLab by @adrien.schiehle on Sep 23, 2021, 03:37 Commented on module/framework.js line 124 changed this line in version 4 of the diff |
In GitLab by @adrien.schiehle on Sep 23, 2021, 03:37 added 1 commit
|
I think we don't need to be scared of making changes if we are in agreement they are good ones. We can save settings for places where we think it's controversial. I would prefer having fewer settings except for things that really need them. |
Let's remove the setting and make this the default behavior. |
Investigate this and see if it leads anywhere useful, I'll stay tuned for your findings. Once this is ready for me to review again please assign me as the reviewer. |
In GitLab by @adrien.schiehle on Sep 23, 2021, 11:18 Commented on module/config.js line 100 changed this line in version 5 of the diff |
In GitLab by @adrien.schiehle on Sep 23, 2021, 11:19 added 1 commit
|
In GitLab by @adrien.schiehle on Sep 23, 2021, 11:19 Commented on module/config.js line 100 That's a good mindset. Parameter removed! |
In GitLab by @adrien.schiehle on Sep 23, 2021, 11:23 Commented on module/config.js line 109 That's a good mindset. Parameter removed. Config panel is simple again : ... for now... 😎 |
In GitLab by @adrien.schiehle on Sep 23, 2021, 11:30 Commented on module/config.js line 100 In fact, the investigation was fast. It works like intended. I removed the includeSidebars and use this article.section for knowing if it should be displayed or not. |
In GitLab by @adrien.schiehle on Sep 23, 2021, 11:35 All which is left is to talk about is module versioning and Readme. For now, I've left the same version : For this type of modification, would a |
resolved all threads |
Looks good! Let's release the change. I will handle the versioning change and release, but I agree with your suggestion we can bump it to |
resolved all threads |
approved this merge request |
In GitLab by @adrien.schiehle on Sep 21, 2021, 04:53
Merges first_step_cosmetics -> master
Some screenshots that will help you see what changed
Default configuration :
Article display with the default configuration :
Custom configuration :
Article display with the custom configuration (GM side) :
Article display with the custom configuration (Player side) :
I didn't update the Readme for this kind of modifications.
Is it necessary ?
The text was updated successfully, but these errors were encountered: