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

Add tests, docs, build #266

Draft
wants to merge 59 commits into
base: master
Choose a base branch
from
Draft

Conversation

TNick
Copy link
Contributor

@TNick TNick commented Oct 15, 2023

This is just a preview to get the discution started.

I has to upgrade ol and ol-ext to be able to run jest.

@TNick TNick marked this pull request as draft October 15, 2023 08:25
@manisandro
Copy link
Member

If I may add one remark right away, I'm a bit skeptical about comments just for the sake of writing a comment but which don't add any real information. In my view they just make the code more cluttered and ultimately less readable.

@TNick
Copy link
Contributor Author

TNick commented Oct 16, 2023

Thanks for the feed-back. Do you have specific examples?

Most of the commetns are written so that they show up in the documentation. I'm in the habbit of interseeding comments before every few lines of code because I find it easier to understand said code a few month after. Especially so with JS. Python is easier on the eyes. My personal experience with open source projects is that the contributors are more likely to contribute if the code is well commented.

That being said I admit that the main reason for writing the comments in code was to give me time to absorb the concepts and to form a mental map of the code.

@manisandro
Copy link
Member

For instance this comment does not really add any meaningful information further than what is already clear from the code:

image

@TNick
Copy link
Contributor Author

TNick commented Oct 26, 2023

Is this the intendend behaviour? Only show the notification if the visibility of a different layer is set.

See the test here:
https://github.com/AdvaitaTopCad/qwc2/blob/8a49f2839ee67107be99a90fbf47d5c83adaaf91/utils/ThemeUtils.test.js#L243

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