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: long code blocks overflow in ui. Fixes #8916 #8947

Merged
merged 1 commit into from
Jun 14, 2022

Conversation

dakotalillie
Copy link
Contributor

Signed-off-by: Dakota Lillie [email protected]

Fixes #8916

This PR addresses an issue where code blocks would overflow their container in the UI.

Before:

Screen Shot 2022-06-10 at 9 30 25 AM

After:

Screen Shot 2022-06-10 at 9 31 24 AM

I have tested this in Chrome, Firefox, and Safari. Safari does have an issue where the scrollbar does not increase the height of the element (notice the lack of padding below the code block):

Screen Shot 2022-06-10 at 10 12 15 AM

Without the hidden overflow-y setting, this would cause an additional vertical scrollbar to appear. I'm inclined to believe this is a bug in Safari, as manually updating any height-related style in the browser, even a random one like "line-height", causes Safari to fix itself:

Screen.Recording.2022-06-10.at.10.16.48.AM.mov

Needless to say, changing the "line-height" in the code does not fix the issue.

I chose to add a scrollbar, rather than wrapping the text, because I find line-wrapped code to be more difficult to read. I can switch approaches if folks would prefer differently, but hopefully this should become a largely moot issue once #8917 is implemented.

I verified to ensure this change does not cause a regression for the bug fixed in #7876. It appears it does not:

Screen Shot 2022-06-10 at 10 34 22 AM

Even with the removal of white-space: pre-wrap, the text still wraps due to overflow-wrap: break-word set here.

I had to disable the pre-commit checks to make this commit, as make test runs a test which attempts to clone a private GitHub repository I don't have access to. I was successfully able to run make pre-commit -B.

@dakotalillie dakotalillie force-pushed the fix-overflow branch 2 times, most recently from 529a14c to 0fa31cd Compare June 10, 2022 17:40
@dakotalillie dakotalillie marked this pull request as ready for review June 10, 2022 19:06
Copy link
Member

@tczhao tczhao left a comment

Choose a reason for hiding this comment

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

LGTM
The PR description is very neat

@alexec alexec enabled auto-merge (squash) June 14, 2022 14:27
@alexec alexec merged commit 6c244f3 into argoproj:master Jun 14, 2022
@dakotalillie dakotalillie deleted the fix-overflow branch June 14, 2022 17:33
@sarabala1979 sarabala1979 mentioned this pull request Jun 20, 2022
55 tasks
sarabala1979 pushed a commit that referenced this pull request Jun 20, 2022
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.

Long container args and/or script source overflows in UI
4 participants