wikitree / wikitree-dynamic-tree

Dynamically generated and browsed graphical family tree
MIT License
23 stars 15 forks source link

Class Names #32

Open bcaseyrls opened 1 year ago

bcaseyrls commented 1 year ago

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.

MichalVasut commented 1 year ago

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

bcaseyrls commented 1 year ago

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.

MichalVasut commented 1 year ago

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

Clarke-11007 commented 1 year ago

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: @.***>

ke4tch commented 1 year ago

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.

MichalVasut commented 1 year ago

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

ke4tch commented 1 year ago

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

bcaseyrls commented 1 year ago

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.

Clarke-11007 commented 1 year ago

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: @.***>

bcaseyrls commented 1 year ago

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.