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 code folding in logger #108 #111

Closed
wants to merge 4 commits into from
Closed

add code folding in logger #108 #111

wants to merge 4 commits into from

Conversation

kagia
Copy link
Contributor

@kagia kagia commented Apr 14, 2016

A new UI element 'foldable' has been added. It allows code folding
for the console. It mirrors the behavior found in most browser
consoles, which is to start collapsed and expand upon selection.

A new UI element 'foldable' has been added. It allows code folding
for the console. It mirrors the behavior found in most browser
consoles, which is to start collapsed and expand upon selection.
@thani-sh
Copy link
Contributor

Looks nice 😃

screen shot 2016-04-14 at 2 24 50 pm

@thani-sh
Copy link
Contributor

@arunoda what do you think?

@kagia
Copy link
Contributor Author

kagia commented Apr 14, 2016

I noticed that we are identifying actions(i.e key={ i }) by their index in the actions list. The problem is that the index is conceptually mutable. React relies on the key prop being conceptually immutable to correctly mount and unmount components. This is why after adding the fifth action there is no visible animation.

I want to update this PR with an extra commit, where actions are being timestamped receive an incremental integer and using that as the identity. Is that okay or shall I open a new issue?

@ritz078
Copy link
Contributor

ritz078 commented Apr 14, 2016

Lot better than present but going forward we should add syntax highlighting and nested folding too. I think highlighting part can be done easily by using highlight.js and passing the string through it.

@arunoda
Copy link
Member

arunoda commented Apr 14, 2016

@andela-bkagia the current version of highlighing doesn't work correctly. So, had to change a bit.
Apply this patch: https://gist.github.com/arunoda/771339c36be6f20892d1e889e35fa412

Also we need to write some tests for this component.
Currently, there are some UI test cases in the master branch.
Try to follow them.

@kagia
Copy link
Contributor Author

kagia commented Apr 15, 2016

@arunoda thanks!
The patch targets compiled output and would easily be overwritten the next time the package is published. Can you send the patch to the original source files?

@arunoda
Copy link
Member

arunoda commented Apr 16, 2016

@andela-bkagia I updated to original patch format, but earlier would also work.
Copy the above patch into a file and simply apply it with.

git apply highlight.patch

@kagia
Copy link
Contributor Author

kagia commented Apr 16, 2016

@arunoda thanks for pointing that out, I have applied the patch and will push the tests later today.

@arunoda
Copy link
Member

arunoda commented Apr 16, 2016

Awesome.

@kagia
Copy link
Contributor Author

kagia commented Apr 16, 2016

I have encountered a problem with testing.

I needed to test that the output of an uncollapsed foldable is a pretty printed document. The tests fail because the first newline encountered is always removed, from the output of calling .text().

I have raised an issue here.

@arunoda
Copy link
Member

arunoda commented Apr 17, 2016

@andela-bkagia that might be the correct behaviour. Just try to avoid it or go around it.
If not, try to write to that test failed with this issue. I'll try to fix.

</span>
</div>

<div className="foldable-content" style={ folderContentStyle }>
Copy link
Member

Choose a reason for hiding this comment

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

Could you remove the className here. May be use a ref. I hope which can be used inside tests.

@kagia
Copy link
Contributor Author

kagia commented Apr 17, 2016

@arunoda I wanted to use refs but I saw that refs are not available with the shallow rendering api.

@arunoda
Copy link
Member

arunoda commented Apr 18, 2016

Okay. Then I try to use JSDom mounting then. I will take this and release today.

@arunoda
Copy link
Member

arunoda commented Apr 18, 2016

I picked this PR and merged to master.
Released as v1.13.0

@arunoda arunoda closed this Apr 18, 2016
@arunoda
Copy link
Member

arunoda commented Apr 18, 2016

Thanks a lot for this.

ndelangen pushed a commit that referenced this pull request Apr 5, 2017
@shilman shilman added the misc label May 27, 2017
Copy link

nx-cloud bot commented Mar 21, 2024

View your CI Pipeline Execution ↗ for commit 739f26c.

Command Status Duration Result
nx run-many -t build --parallel=3 ✅ Succeeded 1m 36s View ↗

☁️ Nx Cloud last updated this comment at 2024-12-30 13:52:43 UTC

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

Successfully merging this pull request may close these issues.

5 participants