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

Add `indented-code-wrapping` feature #1989

Merged
merged 34 commits into from May 24, 2019

Conversation

Projects
None yet
3 participants
@notlmn
Copy link
Contributor

commented Apr 26, 2019

Closes #1908

Uses JS to detect number of spaces at the start of each line, and then uses CSS to move the block forward.

PRs to test

PR conversations and files (dynamically loaded comments, review, and files)

PRs with suggestions

Show resolved Hide resolved source/features/softwrap-code.tsx Outdated
Show resolved Hide resolved source/features/softwrap-code.tsx Outdated

notlmn added some commits Apr 29, 2019

@notlmn

This comment has been minimized.

Copy link
Contributor Author

commented Apr 29, 2019

Temporarily blocked by lukechilds/github-custom-tab-size#3.

@notlmn notlmn marked this pull request as ready for review Apr 29, 2019

@bfred-it

This comment has been minimized.

Copy link
Collaborator

commented Apr 29, 2019

Interesting solutions. Twice on this PR I was like

g9954

@notlmn

This comment has been minimized.

Copy link
Contributor Author

commented Apr 29, 2019

Doesn't exactly show the change in tab size (Gamebar on Windows), but changing value looks something like this:

Show resolved Hide resolved source/features/softwrap-code.tsx Outdated
Show resolved Hide resolved source/features/softwrap-code.tsx Outdated
Show resolved Hide resolved source/features/softwrap-code.tsx Outdated
Show resolved Hide resolved source/features/softwrap-code.tsx Outdated
Show resolved Hide resolved source/features/softwrap-code.tsx Outdated
Show resolved Hide resolved source/features/softwrap-code.tsx Outdated
Show resolved Hide resolved source/features/softwrap-code.tsx Outdated

bfred-it and others added some commits Apr 29, 2019

Apply suggestions from code review
Co-Authored-By: notlmn <notlmn@outlook.com>
@notlmn

This comment has been minimized.

Copy link
Contributor Author

commented Apr 29, 2019

Need to look into performance of the code, test it out more on ajaxed diff tables in PR files and reviews, decide on name of this feature, and can then only this PR should be good to go.

@bfred-it bfred-it changed the title Indentation aware line wrapping Indentation-aware line wrapping Apr 30, 2019

@bfred-it
Copy link
Collaborator

left a comment

A few last changes I think.

In PR Files, files can be loaded asynchronously so we need to listen to that. Look at raw-file-link to see how to implement it. Can you pull out that function into on-pr-file-load.ts?

Show resolved Hide resolved source/features/softwrap-code.tsx Outdated
Show resolved Hide resolved source/features/softwrap-code.tsx Outdated
Show resolved Hide resolved source/features/softwrap-code.tsx Outdated
Show resolved Hide resolved source/features/softwrap-code.css Outdated
Show resolved Hide resolved source/features/softwrap-code.css Outdated
@bfred-it

This comment has been minimized.

Copy link
Collaborator

commented May 2, 2019

Can you commit to maintain this feature going forward? For example, in case GitHub changes something that breaks it. We already have way too many features to maintain, so we need to find a way to make this sustainable.

If so, can you add a line to CODEOWNERS?

notlmn added some commits May 2, 2019

@notlmn

This comment has been minimized.

Copy link
Contributor Author

commented May 2, 2019

Can you commit to maintain this feature going forward? For example, in case GitHub changes something that breaks it. We already have way too many features to maintain, so we need to find a way to make this sustainable.

If so, can you add a line to CODEOWNERS ?

I would be glad to, and yes I added my name to the list. Thanks!

Use `setProperty` on style instead of directly applying the specific …
…style

Because for some unknown reason in this world, using `!important` or overriding and elements `style` which already has `!important` applied doesn't work from JS. Debuggers work, but not JS. :shrug:
Show resolved Hide resolved source/libs/on-pr-file-load.tsx Outdated
Show resolved Hide resolved source/features/indentation-aware-code-wrapping.tsx Outdated
Show resolved Hide resolved source/features/indentation-aware-code-wrapping.tsx Outdated

@bfred-it bfred-it added the enhancement label May 3, 2019

@bfred-it

This comment has been minimized.

Copy link
Collaborator

commented May 12, 2019

I don't get this, can you post a link? Why don't we wrap it?

@notlmn

This comment has been minimized.

Copy link
Contributor Author

commented May 12, 2019

I don't get this, can you post a link? Why don't we wrap it?

That's one of the review comments from the Parcel link above.

We don't wrap it because of the '.blob-code-inner:not(.blob-code-hunk)' selector used. I mean, we could wrap it too, but I don't think that's part of code. Let me know what you think. I'm open to wrap the hunk part too.

@bfred-it

This comment has been minimized.

Copy link
Collaborator

commented May 12, 2019

That part should be wrapped regularly, via CSS. Our JS code shouldn't run on it because it's never indented: its first char is always @.

@bfred-it

This comment was marked as resolved.

Copy link
Collaborator

commented May 13, 2019

Merge with master, it needs a description field because of #2030

notlmn added some commits May 13, 2019

@notlmn

This comment has been minimized.

Copy link
Contributor Author

commented May 13, 2019

Added the description field, but I'd leave out the exact wording to @bfred-it and @sindresorhus. 😉

@bfred-it

This comment has been minimized.

Copy link
Collaborator

commented May 17, 2019

There's a jump we can probably avoid:

This is actually two parts:

  • the line height changes
  • the indentation changes in the suggested code (not a big issue)

This wouldn't be noticeable if there was no delay. Sadly on-new-comments is debounced to 200ms (sadly I don't remember exactly why such a high value) so we have 3 options:

  • add a rgh-softwrapped-code class on body to enable regular wrapping on all elements before the rest of the javascript runs;
  • ensure that the height doesn't change;
  • drop the 200ms delay, after ensuring that all the features that rely on it work correctly on: fresh comments (they're auto-loaded as people comment on issues), collapsed review comments, collapsed comments on long discussions.

Ideally all 3 changes would be made, but probably only one is needed.

@notlmn

This comment has been minimized.

Copy link
Contributor Author

commented May 17, 2019

  • ensure that the height doesn't change;

Done.


  • drop the 200ms delay, after ensuring that all the features that rely on it work correctly on: fresh comments (they're auto-loaded as people comment on issues), collapsed review comments, collapsed comments on long discussions.

I have no idea how changing this would affect other features. Ideally all layout changes should be done under 50ms (just a random value off the top of my head). How about lowering the delay instead of dropping it.

line-height: 21px; /* Compensation for block-ifying table cells */
}

.rgh-code-wrapping-enabled .blob-code-hunk, /* Wrap code eagerly to avoid layout changes later */

This comment has been minimized.

Copy link
@notlmn

notlmn May 17, 2019

Author Contributor
  • add a rgh-softwrapped-code class on body to enable regular wrapping on all elements before the rest of the javascript runs;

This should do the task, but I'm assuming this might affect fragment-targeted-link-loading (changing layout after page load might move the page over the desired view).

@notlmn

This comment has been minimized.

Copy link
Contributor Author

commented May 18, 2019

Is GitHub testing soft-wrapping? I don't remember seeing these CSS classes before.

image

Test PR: sindresorhus/awesome-lint#82

/cc @lukehefson @shayfrendt

@bfred-it

This comment has been minimized.

Copy link
Collaborator

commented May 24, 2019

The line numbers no longer change (61, 62, 63) but the text moves up in Chrome and Firefox (macOS). Maybe this is ok.

3

Avoid layout change
1. Open https://github.com/sindresorhus/refined-github/pull/1881/files
2. Notice minimal layout change when the page stops loading

@bfred-it bfred-it self-assigned this May 24, 2019

Avoid "Suggested change" indentation change
1. Open #1881
2. Expand add-co-authored-by.tsx
3. Notice no indentation change in the "Suggested change" diff

@bfred-it bfred-it removed their assignment May 24, 2019

@bfred-it

This comment has been minimized.

Copy link
Collaborator

commented May 24, 2019

Last note: mixed indentation will break even after the suggested fix a few weeks ago.

If tab-size: 4, \t should not be 3 + 4 characters long but still 4 characters.

Random indentation however should worry us, it should look broken. I think the current implementation still works correctly for "accettable" mixed indentation, like:

 -> -webkit-transform: none;
 -> ••••••••transform: none;

@bfred-it bfred-it merged commit 8dea858 into sindresorhus:master May 24, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bfred-it

This comment has been minimized.

Copy link
Collaborator

commented May 24, 2019

Thanks @notlmn! I think will make the code more readable and is more performant than what I would have come up with 🏎

@notlmn notlmn deleted the notlmn:soft-wrap branch May 24, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.