search

Azure / react-appinsights

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

Adding withAITracking will break styling for components (e.g. full width/height) #73

Open hanvyj opened 5 years ago

hanvyj commented 5 years ago

I just had an issue where I wrapped a component with withAITracking for a component that was scaled to the full width and height of it's parent (width: "100%"; height: "100%").

As the beta now wraps the componet in a div element for the tracking, and this div is unstyled, the resulting wrapped component no longer fills the screen.

hiraldesai commented 5 years ago

Hi @hanvyj - AFAIK it is common for HOC to wrap the original component with a div. I have used it in other projects without it affecting the styles. Is it not something that you can work around by just changing your CSS rules?

I can add a custom class for the wrapper div but I'd rather consumers just worked around it by targeting elements accordingly.

hanvyj commented 5 years ago

You can work around it but it requires you to add some custom styling whever you use a component that's being tracked (with some components anyway).

Seems like it should be the responsibility of the component to style itself rather than rely on anything using it to remember and add some styling.

It just makes me uncomfortable having a situation where I have a reminder "Remember to add this style to the parent of this component or it'll break!" or something.

Ultimately, it's a simple workaround though. Up to you guys of you think it's not something to include? Happy for you to close the ticket if so.

Edit: Although I just had a bug where the 'tracked' component is sometimes in a child, sometimes not - not having the child control the div means the parent's styles .parent > div: { ... } then messes up other children - meaning I have to have either !important or wrap any child in an empty div, feels very hacky.

hiraldesai commented 5 years ago

Can you please advise what is your suggested workaround is? Thanks.

cc @pviotti - FYI.

hanvyj commented 5 years ago

I'm not sure if it would be the best option, but could you just pass through an optional class to set on the div?

withAITracking(component: Component, name?: string, class?: string)

I'll can try do a pull request if I've got some spare time and you think it would be a sensible change?

florianchevallier commented 5 years ago

Hi, I have the same problem here, did you find a way to solve it ?