search

foundryvtt / world-anvil

A module to integrate World Anvil with Foundry Virtual Tabletop.
MIT License
12 stars 7 forks source link

Fix categories - [merged] #46

Closed cswendrowski closed 2 years ago

cswendrowski commented 3 years ago

In GitLab by @adrien.schiehle on Nov 3, 2021, 11:09

_Merges fixcategories -> categories

First commit

For fixing importAll and some hierarchy problems.

We had in fact two Root folder when importing. ( The parent tree node was also a folder named Root )

I decided to let one here. I tested : If they want to remove the upper folder after moving the other one outside of it, it still works. Link remains

Second commit

For visibility buttons. I think that when you tried to simplify things, you found that I didn't use entry.visible and chose to use it.

But its purpose slightly differs from what you thought : WA Panel control is only for GMs, and those buttons gives him info on what is visible by other players? It's great to keep track of what you have shared to your players until now.

It can also be used when you have a NPC who have evolved and its description changed.

(For campaign data, I use a WA category for each act)

cswendrowski commented 3 years ago

In GitLab by @adrien.schiehle on Nov 3, 2021, 11:12

added 1 commit

Compare with previous version

cswendrowski commented 3 years ago

I have merged this code change because it (unfortunately) is too large and touches on too many issues at once for me to effectively review it step-by-step. In the future we need to work on smaller MRs that fixes one issue at a time.

I will leave some comments for things that I think are important to improve upon after this code is merged before we cut a module release.

cswendrowski commented 3 years ago

We should build a module-level cache of categories which persists throughout the user session and is refreshed on-demand rather than specifically passing in categories in this way.

cswendrowski commented 3 years ago

Factoring this method out does not seem to have been a value-adding change since we only use it here and this method already specifically deals with article import.

cswendrowski commented 3 years ago

This isn't a clear/informative name for this const.

cswendrowski commented 3 years ago

We aren't really "refreshing" here so much as re-associating.

cswendrowski commented 3 years ago

We created a separate _sortArticles helper here which is not used anymore. Why?

cswendrowski commented 3 years ago

Why do/should we care that a category is empty?

cswendrowski commented 3 years ago

What does it mean in this context for an article not to be "visible", we receive the data from the WA API so why would it not be "visible"?

cswendrowski commented 3 years ago

Is this change done in order to import all articles under a parent category recursively? If so... that is a useful feature to add but new features like this should not be part of a MR like this. It becomes way too much to review at once. We should add these feature changes one at a time.