MLH-Fellowship / scheduling-profiler-prototype

Custom profiler prototype for React's concurrent mode
https://react-scheduling-profiler.vercel.app/
6 stars 0 forks source link

Fix tooltip #121

Closed taneliang closed 4 years ago

taneliang commented 4 years ago

Fixes #87, and refixes #47 with a solution that solves its root cause.

Details and summary of changes in commit messages.

Test Plan

vercel[bot] commented 4 years ago

This pull request is being automatically deployed with Vercel (learn more). To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/mlh-fellowship/scheduling-profiler-prototype/10kzjnap5 ✅ Preview: https://scheduling-profiler-prototype-git-eliang-fix-tooltip.mlh-fellowship.vercel.app

kartikcho commented 4 years ago

Allow display of long script URLs

This was fixed before too, I can't seem to repro this.

image

Script URLs now limited in height to 50vh

I think truncating script URL length is a better choice visually than setting max-height to 50vh with overflowing text, if we want to limit URL length at all.

Also, the tooltip doesn't seem to limit height at 50vh. I wasn't able to inspect the div but I hovered over the same cell and it didn't limit height.

image

Disable truncation of React component names

I think you should add that back in because,

taneliang commented 4 years ago

This was fixed before too, I can't seem to repro this.

Seems like Firefox behaves differently from Chrome.

I think truncating script URL length is a better choice visually than setting max-height to 50vh with overflowing text, if we want to limit URL length at all.

The goal was actually to prevent the entire tooltip from getting too tall and stretching offscreen, not to limit the URL length. What do you think? It's definitely a bit of a hack. We can also limit the URL length but a hardcoded length will not work for all viewport heights, and too low a limit may obscure information.

it didn't limit height.

How tall was your viewport?

It's still a case and we should have it covered

Sounds reasonable. I'll limit it to 768 chars then. I can now think of a legitimate-ish case for having long component names: a component can be wrapped in multiple HoCs. And if that's the case, then limiting to 128 chars will not be enough for the user to identify the component that was wrapped. This char limitation will only be there so that the page layout doesn't get all wonky, but I don't expect that limit to be hit. Ideally we'll want to get some metrics to verify this, but I think in the absence of that this high limit should work well enough.

This screenshot shows a 768-char component name generated with this code snippet:

  const componentName = [...Array(768)]
    .map(() => Math.random().toString(36)[2])
    .join('');

image

FB profile has long component names in many places, you might not have encountered them.

Could you point me to it? The longest component name I found in that profile was 84 chars long.

kartikcho commented 4 years ago

a hardcoded length will not work for all viewport heights, and too low a limit may obscure information.

That's a good reason not to limit it by number of characters, we should go with height (although the overflow makes the last line sometimes half covered which doesn't look so good).

How tall was your viewport?

1080 pixels. I realized it was too tall to hide URL. I tested with a smaller screen and it works.

I'll limit it to 768 chars then.

Your reasoning seems valid, why specifically 768 though? A limit of 512 seems more reasonable for a component name. Other than that, sounds good!

taneliang commented 4 years ago

why specifically 768 though

No particular reason for that particular number. I just want as high a limit as possible for the reasons mentioned above. I tried 1000 chars at first, but that was too tall.

taneliang commented 4 years ago

although the overflow makes the last line sometimes half covered which doesn't look so good

Agree. I don't have a good solution for that, but I think it's acceptable.

kartikcho commented 4 years ago

No particular reason for that particular number. I just want as high a limit as possible for the reasons mentioned above. I tried 1000 chars at first, but that was too tall.

Maybe 768 would be too tall too for a viewport like that of the Devtools I think. We can iterate when it comes to that though, for now it's a good decision.