search

bloomreach / spa-sdk

Apache License 2.0
17 stars 14 forks source link

ng-sdk, BrPageService does not support multiple br-page components #13

Closed Meightem closed 1 year ago

Meightem commented 1 year ago

With the introduced changes in commit 88450a817171f84efe94bc51dc005f5575858e46 there can only be one br-page at a time.

Is there any reason that the state of the br-page component was extracted into a service which is injected at root level? This breaks my application as I have multiple br-pages at the same time and therefore, state from one br-page sets the state of the other.

I would suggest removing the injection at root level and instead provide it for each instance of the br-page. I have also created a pull request to further illustrate my problem and possible solution. See my pull-request #12

beetlerom commented 1 year ago

Thank you for reporting this issue @Meightem! Appreciate the MR, but I kindly ask for a bit of patience (somewhere second week of March) as we are working to make the GitHub repository our main repository (currently this is a clone) and we could then directly accept your contribution.

@joerideg Adding this to the list of improvements in v18+

antoniodesousa commented 1 year ago

Hi @Meightem, I'm sorry to hear that you are having issues with the latest release of the Angular SDK. The state was extracted into a service because the library had circular dependencies between several components, which is not allowed in a library. I wasn't aware that several pages could be rendered at the same time that's why I added the service to the root level. I will change this behavior as soon as possible. Thanks for reporting this issue.

Meightem commented 1 year ago

Hi,

Thank you for your quick responses.

@beetlerom, if it helps to get the fix out more quickly, you can fix the issue in your current main repository. You don't need to use my commit specifically. I only created an MR to suggest a possible solution. We currently use patch-package to fix this issue, so we don't need an immediate public fix.

Thank you, @antoniodesousa, for clarifying the reasoning behind that change. It makes sense to me. The use case is that we have a side-menu rendered with a br-page to navigate, and the main content also rendered in a separate br-page. Is there another/better way to implement this without two br-pages? I am not familiar with all the Bloomreach functionality.

antoniodesousa commented 1 year ago

Hi @Meightem, the release 19.0.2 adds the support you wanted for your project. I hope that everything works as expected now.

Is there another/better way to implement this without two br-pages?

That's a good question, sadly I wouldn't know.

antoniodesousa commented 1 year ago

I gonna close the issue as resolved.