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

Embedded editor improvements #4936

Merged
merged 9 commits into from
Jan 2, 2024
Merged

Embedded editor improvements #4936

merged 9 commits into from
Jan 2, 2024

Conversation

TiBiBa
Copy link
Collaborator

@TiBiBa TiBiBa commented Dec 27, 2023

Description
In this PR we make some improvements to the embedded editor. The editor itself was updated recently to be better responsive, but the embedded editor was left out-of-scope. We make some improvements to the scale-ability, fixing some overflow with long lines of code and automatically grow the editor to the available height.

Fixes
This has no related issue

Example
Old
Scherm­afbeelding 2023-12-27 om 10 02 37
Fixed
Scherm­afbeelding 2023-12-27 om 10 02 44

How to test
http://127.0.0.1:8080/embedded/1/?lang=nl&run=true&readOnly=false&program=cHJpbnQgSG9pLCB3ZWxrb20gYmlqIEhlZHkhIEhvaSwgd2Vsa29tIGJpaiBIZWR5ISBIb2ksIHdlbGtvbSBiaWogSGVkeSEgSG9pLCB3ZWxrb20gYmlqIEhlZHkhIEhvaSwgd2Vsa29tIGJpaiBIZWR5IQphc2sgSG9lIGdhYXQgaGV0IG1ldCBqZT8KXyBEdXMgaGV0IGdhYXQ=

@ghost
Copy link

ghost commented Dec 27, 2023

👇 Click on the image for a new way to code review

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map legend

@TiBiBa TiBiBa self-assigned this Dec 27, 2023
@TiBiBa TiBiBa requested a review from jpelay December 27, 2023 08:59
@jpelay
Copy link
Member

jpelay commented Dec 27, 2023

Seems to work great!

@TiBiBa TiBiBa marked this pull request as ready for review December 27, 2023 16:58
Copy link
Member

@jpelay jpelay left a comment

Choose a reason for hiding this comment

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

There's one problem though, we need to set a value for the height of the editor as well. Otherwise it keeps growing and eats the space for the run button:

image

@TiBiBa
Copy link
Collaborator Author

TiBiBa commented Dec 27, 2023

There's one problem though, we need to set a value for the height of the editor as well. Otherwise it keeps growing and eats the space for the run button:

image

I'm unable to re-produce this issue. The min-h-0 class should fix that the button container is always visible. Will look into this tomorrow how to fix.

@TiBiBa
Copy link
Collaborator Author

TiBiBa commented Dec 27, 2023

I tried it with an iframe of just 200px height but it still renders fine:
Scherm­afbeelding 2023-12-27 om 19 57 00

Do you have an example iframe that breaks the editor?

@TiBiBa TiBiBa requested a review from jpelay December 28, 2023 09:16
@TiBiBa
Copy link
Collaborator Author

TiBiBa commented Dec 28, 2023

Hi @jpelay! I think the described issue is fixed. Was a bit of "puzzelen", but all should work now!

@jpelay
Copy link
Member

jpelay commented Dec 29, 2023

Hi, Timon! Sorry that I wasn't clear enough in my comment, here I add a video about the problem:

2023-12-29.10-12-12.mp4

Here you can see that after adding enough lines, the editor surpasses the height, and there's no scrollbar, the only way to see the previous lines is by pressing the up arrow. Maybe this is just a case of adding an overflow? Also, as you can see the horizontal scrollbar is not shown in the bottom of the editor area, but rather right bellow the content.

@TiBiBa
Copy link
Collaborator Author

TiBiBa commented Jan 2, 2024

Hi, Timon! Sorry that I wasn't clear enough in my comment, here I add a video about the problem:

2023-12-29.10-12-12.mp4

Here you can see that after adding enough lines, the editor surpasses the height, and there's no scrollbar, the only way to see the previous lines is by pressing the up arrow. Maybe this is just a case of adding an overflow? Also, as you can see the horizontal scrollbar is not shown in the bottom of the editor area, but rather right bellow the content.

Thanks for the video! Will try to figure this out.

@TiBiBa
Copy link
Collaborator Author

TiBiBa commented Jan 2, 2024

@jpelay This should finally be fixed, please let me know if it also works for you!

Copy link
Member

@jpelay jpelay left a comment

Choose a reason for hiding this comment

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

Seems to be working perfectly now! Thanks for the hard work, Timon!

Copy link
Contributor

mergify bot commented Jan 2, 2024

Thank you for contributing! Your pull request is now going on the merge train (choo choo! Do not click update from main anymore, and be sure to allow changes to be pushed to your fork).

@mergify mergify bot merged commit be4c2ef into main Jan 2, 2024
@mergify mergify bot deleted the fix-embedded-editor branch January 2, 2024 15:53
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.

2 participants