Skip to content
This repository has been archived by the owner on Sep 30, 2024. It is now read-only.

Fix chart label in Firefox for code insight pie chart #20120

Merged
merged 3 commits into from
Apr 19, 2021

Conversation

vovakulikov
Copy link
Contributor

Fixes: https://github.com/sourcegraph/sourcegraph/issues/20084

I consider this problem came from how Firefox treats empty svg elements without width and height within another svg element with height and width - They usually have width=100% height=100% of parent size even if we put size value from CSS width: 0px; height: 0px; explicitly.

In the first render we're getting a really big value of Text svg height, so we're just stuck with this big initial height value here and therefore have this visual behaviour here.

Possible solution in this PR this is replace innerRef target from svg <Text /> root element to <text /> element which can be measured without this problem.

Problem is Text component comes from @visx/text package. Just not to be blocked by this fixes inside visx packages. I forked this component and added this fix with innerRef. But we have to pull origin component when this problem will be resolved at visx codebase.

@vovakulikov vovakulikov added webapp code-insights Issues related to the Code Insights product labels Apr 19, 2021
@vovakulikov vovakulikov requested review from valerybugakov and a team April 19, 2021 06:21
@vovakulikov vovakulikov self-assigned this Apr 19, 2021
@felixfbecker
Copy link
Contributor

I think we should merge this as a quick fix so we don't ship broken charts for Firefox on the 20th.

@felixfbecker felixfbecker added the release-blocker Prevents us from releasing: https://about.sourcegraph.com/handbook/engineering/releases label Apr 19, 2021
@vovakulikov vovakulikov merged commit 5ad7014 into main Apr 19, 2021
@vovakulikov vovakulikov deleted the vk/bad-annotation-chart-label-in-firefox branch April 19, 2021 10:59
@vovakulikov
Copy link
Contributor Author

vovakulikov commented Apr 19, 2021

@felixfbecker @valerybugakov My apologies for merge this without review. I was experimenting with PR's flow in IntelliJ IDEA and by accident merged this one.

I tested this changes manually everything works as expected
image

Most changes of this PR is about text component. All other things are just pulled changes from main branch of visx packages.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
code-insights Issues related to the Code Insights product release-blocker Prevents us from releasing: https://about.sourcegraph.com/handbook/engineering/releases webapp
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Code Insights: Bad annotation label at Firefox
2 participants