Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(code-viewer): get rid of trailing lines #188

Merged
merged 7 commits into from
Aug 17, 2020

Conversation

P0lip
Copy link
Contributor

@P0lip P0lip commented Aug 11, 2020

@P0lip P0lip added bug Something isn't working team/void-crew labels Aug 11, 2020
@P0lip P0lip added this to the 2020-08-05 milestone Aug 11, 2020
@P0lip P0lip requested a review from a team August 11, 2020 21:51
@P0lip P0lip self-assigned this Aug 11, 2020
@lottamus
Copy link
Contributor

lottamus commented Aug 14, 2020

I yalced this into platform-internal and it's still doing a weird jump

2020-08-14 09 46 09

And also not highlighting all of the lines

2020-08-14 09 47 09

For comparison, here's how the indentation looks in production

image

@P0lip @wmhilton how about we switch to an open source code highlighter like https://github.com/react-syntax-highlighter/react-syntax-highlighter

@P0lip
Copy link
Contributor Author

P0lip commented Aug 14, 2020

@lottamus
the jump from left to right is on purpose.
We adjust the size of sidebar so that all numbers can fit.
Previously, we used to have a pre-defined width, but this is no longer the case.
Right now, the width is adjusted to the number of digits.

And also not highlighting all of the lines

bahhhh, let me see why is this happening. 🙄

@lottamus
Copy link
Contributor

We adjust the size of sidebar so that all numbers can fit.

I think we might be over adjusting in this case. I'm pasting in the platform-internal root package.json which only goes up to 3 digits. Maybe start the adjustment if we're at 5+ digits. Or even larger, I'd rather our default styling handles a majority of the use-cases so most users never see the jumping (unless they're looking at something juuuuuge)

@P0lip
Copy link
Contributor Author

P0lip commented Aug 14, 2020

Speaking of code highlighting issue - I know why it's happening.
It's because we currently listen to window and root scroll events, but in case of Studio we have that fancy custom scrollbar.

I think we might be over adjusting in this case. I'm pasting in the platform-internal root package.json which only goes up to 3 digits. Maybe start the adjustment if we're at 5+ digits. Or even larger, I'd rather our default styling handles a majority of the use-cases so most users never see the jumping (unless they're looking at something juuuuuge)

Yeah, that makes sense. Will do.

Copy link
Contributor

@billiegoose billiegoose left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It actually works quite well this time. At least in my testing in Storybook. I'll try it yalced into Studio next.

@billiegoose
Copy link
Contributor

@P0lip @wmhilton how about we switch to an open source code highlighter like https://github.com/react-syntax-highlighter/react-syntax-highlighter

@lottamus Well, react-syntax-highlighter uses the exact same library (refractor) that we do, so I doubt it would be able to handle these large files. I can throw together a POC to compare though.

if (maxLines === null) {
return null;
}

const blocks: string[] = [''];
const blocks: SlicedBlock[] = [createSlicedBlock()];

for (let i = 0, n = 0; i < value.length; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image
image

Copy link
Contributor

@billiegoose billiegoose left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When yalced into Studio it broke the Try It component.

@P0lip
Copy link
Contributor Author

P0lip commented Aug 17, 2020

@wmhilton do yo uhave the latest elements installed?
I fixed that issue in elements. It's not codeviewer's fault. It's TryIt component using safeStringify that returns undefined when you pass undefined.

@billiegoose
Copy link
Contributor

Ahhh. So I gotta upgrade both at once huh? :eyeroll: OOOOOk lemme try that.

@P0lip
Copy link
Contributor Author

P0lip commented Aug 17, 2020

Copy link
Contributor

@billiegoose billiegoose left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works perfectly.

@billiegoose billiegoose merged commit b9599f9 into beta Aug 17, 2020
@billiegoose billiegoose deleted the fix/code-viewer-trailing-lines branch August 17, 2020 21:19
@stoplight-bot
Copy link
Collaborator

🎉 This PR is included in version 3.0.0-beta.32 🎉

The release is available on:

Your semantic-release bot 📦🚀

@billiegoose
Copy link
Contributor

@lottamus Well, react-syntax-highlighter uses the exact same library (refractor) that we do, so I doubt it would be able to handle these large files. I can throw together a POC to compare though.

@lottamus Here's you answer:

Firefox:
image

Chrome:
2020-08-17 17 24 51

At the time I post this, my Chrome tab still hasn't recovered. My fans are spinning like crazy. Google Chrome Helper (Renderer) is using 100% CPU.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants