Skip to content

Conversation

@qFamouse
Copy link

Changes

Updating the framework

Angular has been updated to the latest stable version (v15) e95ad62

Bug fix (#8)

This problem was related to the @Input settings.
In the input of the map, we initialize the sidebar, but in this case, the options field cannot be set by the user in this context. Therefore, when creating a sidebar, we take the value set by the constructor of our component ({} is an empty object).
When initializing the sidebar with an empty object, we can see the problem described in (#8)

I solved this problem by transferring the creation of the sidebar after setting the properties of the component (OnInit)

Refactoring

It seemed to me that the approaches written in the code were outdated. Therefore, I suggest changing the written code.

  • I removed get/set from @input() map
  • I looked the leaflet-sidebar-v2 source code and found that the 'id' that you declare here is added only at the content event. You create your own SidebarEvent interface and say that all events return an id, although this is not the case.
    • I removed the Leaflet Event interface because it seems to me redundant. But I think it's really important to let programmers know that 'content' returns id, so I combined LeafletEvent and id.
  • I was horrified by subscribing to 3 different events through one change$ emitter.
    • I made 3 separate emitters for each of the events.

    • Now usings
      image

    • I did not add the $ sign to the names of these emitters (as you have change$), because now, according to convention, observable types are designated this way.

Conclusion

I started to figure it out, because no one solved my problem (#8) :) When I had free time, I decided to do it.

Fixing this problem, I learned a lot of new and interesting things related to the work of libraries loaded on npm. For me as a student, it was a useful experience)

I hope you will like my corrections)

@runette
Copy link
Owner

runette commented Feb 14, 2023

Hi

Thank you for the PR. This is very much the spirit of open source - you find a problem, you fix the problem, you pay the solution forward to the community and hopefully in the process you learn something new.

That said - I have some issues with the PR. I will list most of them here and then we can get into detailed code review :

  1. Please DO NOT change the Angular version and package.json. This is not working in the way you appear to think it is working ! The package is published using Ivy AOT Partial Compilation as described in this article - to be simple it is published through a pre-compiler but the actual compilation (or more correctly transpilation) is done in the consuming application module based on the Angular version in that module. The current configuration allows the library to be published for use by any Angular version after Version 12 (including Version 15) without change and without the annoying habit of libraries chasing Angular versions and applications chasing Library versions.

Please revert all changes to package.json in the PR .

  1. I am not sure what is going on with the wholesale change to .gitignore and I don't have the time to work it out. Please don't make wholesale changes without a detailed explanation and please revert the changes to .gitignore.

  2. I cannot remember why the three events were originally in one struct. Your proposal is better - if it was an addition. However, you have made it a breaking change - i.e. not backwardly compatible. Please refactor as a non-breaking change.

  3. You have changed the initiation to OnInit.You are dependent on the map being instantiated before OnInit - which I can guarantee will not work in most cases. There is a reason why this was done as it is. Please explain how this is intended to work if the map has not been instantiated at the OnInit stage. I also still do NOT understand how you are getting your current problem - there is no way that the directive can be called before the Component Properties are set!

Please provide more details.

@qFamouse
Copy link
Author

Hi. Thanks for the reply. I realized my mistakes, thank you. As soon as there is time to correct my mistakes, I will come back to you with a review)

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

Successfully merging this pull request may close these issues.

2 participants