wikitree-dynamic-tree icon indicating copy to clipboard operation
wikitree-dynamic-tree copied to clipboard

Class Names

Open bcaseyrls opened this issue 3 years ago • 10 comments

I noticed that both the FanChart view and the Ahnentafal view had an "Ahnentafel" class. I was concerned about there being more than one window.Ahnentafel. Both views seemed to work, but I was seeing some odd problems during integration into WikiTree.com and thought it wouldn't hurt to change this. I modified the class name in the Ahnentafel view (to be "AhnentafelAncestorList").

Should we have some guidelines (perhaps in codestyle.mld - https://github.com/wikitree/wikitree-dynamic-tree/issues/31) about naming? Everything is small enough now that it's not a big issue, but moving forward it might become one.

bcaseyrls avatar Oct 11 '22 15:10 bcaseyrls

Hmm yeah, this could be problem in the future (or already is). Naming guidlines are one way to deal with it, the second one is to use modules ( docs for Modules & CanIUse page ) and import / export only what is needed, so it'll stay safely in the file / module context till we expose it to the outside world.

But I've never worked with those. 😢 As per Can I Use page it should be safe to use (supported by major browsers).

MichalVasut avatar Oct 11 '22 16:10 MichalVasut

I think we should just start with some guidelines. That'll take us a long way. If a new view developer wants to use Modules, then that will be a good example for others to follow in the future.

bcaseyrls avatar Oct 11 '22 16:10 bcaseyrls

I'll be making Printer friendly view so I'll try to be pioneer in using of this approach 🤔

MichalVasut avatar Oct 11 '22 16:10 MichalVasut

HI there Brian - yes - I saw that as well - and was worried, but the different cases (you'll notice that I used AhnenTafel instead of all lowercase) seemed to be enough to allow this duplication (at least locally as I was testing it).

Since my AhenTafel.js file is purely a class - and not a view - I was thinking yours might be renamed AhnentafelView - but - you've already done something like that.

I like the idea of naming the classes as the collective objects or single objects they represent - hence PeopleCollection (and AhnenTafel which literally translates as Ancestor Table - I guess I could have just used that instead ! DOOH ) - and - then the View or List or Trees or Charts have those words at the end of their names.

That's my initial two cents on the matter.

Thanks again -Greg :-)

On Tue, Oct 11, 2022 at 12:16 PM Michal Vašut @.***> wrote:

I'll be making Printer friendly view so I'll try to be pioneer in using of this approach 🤔

— Reply to this email directly, view it on GitHub https://github.com/wikitree/wikitree-dynamic-tree/issues/32#issuecomment-1274949902, or unsubscribe https://github.com/notifications/unsubscribe-auth/A3JDGBW2MT3MUISNXBM5JWLWCWHGHANCNFSM6AAAAAARCN7D6A . You are receiving this because you are subscribed to this thread.Message ID: @.***>

Clarke-11007 avatar Oct 11 '22 17:10 Clarke-11007

Class and file naming conventions and coding style guidelines probably belong in a wikitree-common package, along with things like.prettier.rc, lint stuff and shared code. Then we need guidelines for lazy loading and chunking.

BioCheck has some common class names (Person, PersonDate, PeopleManager, Biography) which are all in modules.

ke4tch avatar Oct 15 '22 13:10 ke4tch

wikitree-common package would make sense, even for other things like date parsing and human format serialization (it is implemented in every view again and again), but I wasn't aware that this package exists. Or maybe it's only out of my reach (not opensourced on Github)? 🤔

MichalVasut avatar Oct 15 '22 14:10 MichalVasut

I just refactored the https://github.com/ke4tch/wikitree-biocheck/blob/main/src/Person.js and companion https://github.com/ke4tch/wikitree-biocheck/blob/main/src/PeopleManager.js classes in the hope of reuse. The PersonDate that Person extends is already copied/replicated between the standalone app and the browser extension

ke4tch avatar Oct 15 '22 21:10 ke4tch

There is no currently-existing "wikitree-common" package or repo. Sharing some common utility functions/classes across views (and across the dynamic tree / browser extension projects) would be helpful. I'm not sure the best way to do that. We could create a separate wikitree-common repo, but I confess I still find GitHub "submodules" a bit confusing and I'm wary of causing havoc by trying to set that up.

bcaseyrls avatar Oct 16 '22 16:10 bcaseyrls

What if we had a directory call /common in each repo, and out the files there? I have classes that live in my FanChart directory but are used in two other views, so a /common place to store them would make more sense

This of course still means there will be work to ensure views and extension /common folders stay in sync, or at least the classes in common!

On Sun, Oct 16, 2022 at 12:43 PM Brian Casey @.***> wrote:

There is no currently-existing "wikitree-common" package or repo. Sharing some common utility functions/classes across views (and across the dynamic tree / browser extension projects) would be helpful. I'm not sure the best way to do that. We could create a separate wikitree-common repo, but I confess I still find GitHub "submodules" a bit confusing and I'm wary of causing havoc by trying to set that up.

— Reply to this email directly, view it on GitHub https://github.com/wikitree/wikitree-dynamic-tree/issues/32#issuecomment-1280006105, or unsubscribe https://github.com/notifications/unsubscribe-auth/A3JDGBW26V4CTZYKZD2ZVI3WDQWDRANCNFSM6AAAAAARCN7D6A . You are receiving this because you commented.Message ID: @.***>

Clarke-11007 avatar Oct 17 '22 01:10 Clarke-11007

I think a /common or /shared directory, either from the root or inside views/, makes sense. JS code/class files that can be used by multiple views could be put in there, instead of residing inside a specific view's directory.

bcaseyrls avatar Oct 25 '22 22:10 bcaseyrls