Cosmetics modifications - [merged]

[cswendrowski]

In GitLab by @adrien.schiehle on Sep 21, 2021, 04:53

_Merges first_stepcosmetics -> 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 ?

[cswendrowski]

Convention: localization keys are title case, for example "WA.IncludeSidebarsLabel"

[cswendrowski]

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?

[cswendrowski]

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?

[cswendrowski]

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)?

[cswendrowski]

this setting makes sense to me, but I think the setting name chosen here is a bit difficult to understand/unintuitive. Perhaps something like publicArticleLinks (default false)? Feel free to suggest other alternative names, but I think linkOutsideGMs is not great.

[cswendrowski]

Nitpick, call this variable something like isLongContent which (1) makes it clear it's a boolean and (2) checks length rather than some measure of "bigness".

[cswendrowski]

Prefer using an early continue to avoid an extra level of if clause:

if ( !section.items ) continue;

[cswendrowski]

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.

[cswendrowski]

Thanks for your submission, this looks pretty good! I have a couple questions to get on the same page about.

[cswendrowski]

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

[cswendrowski]

In GitLab by @adrien.schiehle on Sep 23, 2021, 01:59

added 1 commit

• 0e0ca464 - Convention: localization keys are title case

Compare with previous version

[cswendrowski]

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

[cswendrowski]

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

[cswendrowski]

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

[cswendrowski]

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

[cswendrowski]

In GitLab by @adrien.schiehle on Sep 23, 2021, 03:24

added 1 commit

• a08a1f0a - includeSidebars retrieved from WA

Compare with previous version

[cswendrowski]

In GitLab by @adrien.schiehle on Sep 23, 2021, 03:30

Commented on lang/en.json line 7

Good to know. Fixed.

[cswendrowski]

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.

So I chose to totally remove them from the import.

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 Displaysidebar, is always here, and its content is 0/1.

I can try to base my import on that instead.

I will need 4 labels, one for each sidebar title. For now, only one is renamed as 'General Details'. For now, I will put 'General Details' for each one.

• "Sidebarcontent" (top sidebar)
• "Sidepanelcontenttop" (top content panel)
• "Sidepanelcontent" (bottom content panel)
• "Sidebarcontentbottom" (bottom sidebar)

[cswendrowski]

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)

I will gladly remove it if you think it won't pose problem with those who already use the module

[cswendrowski]

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.

[cswendrowski]

In GitLab by @adrien.schiehle on Sep 23, 2021, 03:31

Commented on module/config.js line 128

I'm ok with publicArticleLinks. I've always been bad with var naming :angel:

[cswendrowski]

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.

[cswendrowski]

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

[cswendrowski]

In GitLab by @adrien.schiehle on Sep 23, 2021, 03:37

added 1 commit

• 3f02dd03 - continue instead of if clause

Compare with previous version

[cswendrowski]

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.

[cswendrowski]

Let's remove the setting and make this the default behavior.

[cswendrowski]

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.

[cswendrowski]

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

[cswendrowski]

In GitLab by @adrien.schiehle on Sep 23, 2021, 11:19

added 1 commit

• 34b76497 - repeatTitle & linkOnHeader conf removed

Compare with previous version

[cswendrowski]

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!

[cswendrowski]

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... :sunglasses:

[cswendrowski]

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.

[cswendrowski]

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 : 1.0.3

For this type of modification, would a 1.1.0 be ok ?

[cswendrowski]

resolved all threads

[cswendrowski]

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 1.1.0

[cswendrowski]

resolved all threads

[cswendrowski]

approved this merge request