Skip to content
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

Class Names #32

Open
bcaseyrls opened this issue Oct 11, 2022 · 10 comments
Open

Class Names #32

bcaseyrls opened this issue Oct 11, 2022 · 10 comments

Comments

@bcaseyrls
Copy link
Contributor

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 - #31) about naming? Everything is small enough now that it's not a big issue, but moving forward it might become one.

@MichalVasut
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

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

@Clarke-11007
Copy link
Contributor

Clarke-11007 commented Oct 11, 2022 via email

@ke4tch
Copy link
Contributor

ke4tch commented Oct 15, 2022

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
Copy link
Contributor

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
Copy link
Contributor

ke4tch commented Oct 15, 2022

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
Copy link
Contributor Author

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
Copy link
Contributor

Clarke-11007 commented Oct 17, 2022 via email

@bcaseyrls
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants