search

Azure / react-appinsights

Typescript library to use Application Insights in React applications [deprecated, see microsoft/ApplicationInsights-JS]
MIT License
78 stars 32 forks source link

Added a classname for the appInsights HOC's div #89

Closed florianchevallier closed 5 years ago

florianchevallier commented 5 years ago

Just a way to fix #73

florianchevallier commented 5 years ago

@pviotti could you please check this PR ? Thank you

pviotti commented 5 years ago

Hi, sorry for the late reply. This project has been officially "adopted" by the AppInsights team, so the next version will be hosted in the ApplicationInsights-JS mono-repo, and it's currently available at this path: https://github.com/microsoft/ApplicationInsights-JS/tree/master/vNext/extensions/applicationinsights-react-js Soon we will be closing this project, deprecating the package and redirecting people to that repository.
So I suggest to port the changes of this PR to the code in that repository.

Regarding this PR, I'm not sure what you actually aim to solve by adding an arbitrary class name to the HoC div...

florianchevallier commented 5 years ago

Hi, thanks for your answer and the clarification concerning the future of this repo.

Regarding this PR, giving an arbitrary class name to the HoC div helps solving this probleme ( #73 ). In our app, the app has a 100% height / width on the divs, but adding another div as a HoC and not beeing able to style it easily breaks the layout of my app. If you find a way to add a dynamic class name to the HoC, it would be great, but for now, adding this name will be really enough.

pviotti commented 5 years ago

OK, understood. I think that adding an optional argument to the HoC function would be a cleaner solution, as proposed in this comment: https://github.com/Azure/react-appinsights/issues/73#issuecomment-479137008 I'll be closing this PR, but I encourage you to propose this change to the new codebase. Thanks!

florianchevallier commented 5 years ago

OK, understood. I think that adding an optional argument to the HoC function would be a cleaner solution, as proposed in this comment

I agree on that, but since the 2nd argument is already a component name, it will force you to use a name even if you don't need one.

I'll open the PR on the new codebase. Thank you