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.

Sign up for GitHub

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

Jump to bottom

Added option to wrap lines in EuiCodeBlock #3103

Merged
merged 20 commits into from
Mar 23, 2020
Merged

Added option to wrap lines in EuiCodeBlock #3103

merged 20 commits into from
Mar 23, 2020

Conversation

anishagg17
Copy link
Contributor

@anishagg17 anishagg17 commented Mar 17, 2020 •
edited
Loading

Summary

Fixes : #1501

Screenshot

wrapLines : nowrap

Hnet com-image (8)

Checklist

- [ ] Check against all themes for compatibility in both light and dark modes
- [ ] Checked in mobile

  • Checked in IE11 and Firefox
  • Props have proper autodocs
  • Added documentation examples
  • Added or updated jest tests
    - [ ] Checked for breaking changes and labeled appropriately
    - [ ] Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

@kibanamachine
Copy link

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@cchaos cchaos changed the title added option to wrap lines in euicodeblock Added option to wrap lines in EuiCodeBlock Mar 17, 2020
@elizabetdev elizabetdev self-requested a review March 17, 2020 13:12
@elizabetdev
Copy link
Contributor

jenkins test this

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_3103/

snide
snide suggested changes Mar 17, 2020
View reviewed changes
Copy link
Contributor

@snide snide left a comment

Choose a reason for hiding this comment

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

Couple changes. Thanks for taking on this issue.

src/components/code/_code_block.tsx Outdated Show resolved Hide resolved
elizabetdev
elizabetdev suggested changes Mar 18, 2020
View reviewed changes
Copy link
Contributor

@elizabetdev elizabetdev left a comment

Choose a reason for hiding this comment

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

Thanks for creating this PR. I tested locally and I added a few comments.

Also, don't forget to go through our checklist. It's very important to make sure it works on IE11 and Firefox. 🙂

src/components/code/_code_block.scss Outdated Show resolved Hide resolved
src/components/code/_code_block.tsx Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
anishagg17 and others added 3 commits March 18, 2020 17:36
@anishagg17 @elizabetdev
Update src/components/code/_code_block.scss
62f7d61 
Co-Authored-By: Elizabet Oliveira <elizabet.oliveira@elastic.co>
@anishagg17 @elizabetdev
Update src/components/code/_code_block.tsx
ac9134c 
Co-Authored-By: Elizabet Oliveira <elizabet.oliveira@elastic.co>
@anishagg17 @elizabetdev
Update CHANGELOG.md
e6c2845 
Co-Authored-By: Elizabet Oliveira <elizabet.oliveira@elastic.co>
@anishagg17
Copy link
Contributor Author

Thanks @miukimiu

@elizabetdev
Copy link
Contributor

Hi @anishagg17,

I just did a PR with a few changes anishagg17#5.

  • The CSS classes should respect the BEM naming convention.
  • I also changed the description text and added some code highlights

Can you approve the PR?

elizabetdev
elizabetdev approved these changes Mar 18, 2020
View reviewed changes
Copy link
Contributor

@elizabetdev elizabetdev left a comment

Choose a reason for hiding this comment

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

Thanks @anishagg17! LGTM! 🎉

@elizabetdev elizabetdev requested a review from cchaos March 18, 2020 16:28
cchaos
cchaos reviewed Mar 18, 2020
View reviewed changes
Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

Looking better, but still needs a test and a doc example for showing the non-default case of pre.

@anishagg17
Copy link
Contributor Author

I will add It

@anishagg17
Copy link
Contributor Author

All changes have been made

@cchaos
Copy link
Contributor

cchaos commented Mar 20, 2020

Jenkins test this

elizabetdev
elizabetdev suggested changes Mar 20, 2020
View reviewed changes
src-docs/src/views/code/code_block_pre.js Outdated Show resolved Hide resolved
src/components/code/_code_block.test.tsx Outdated Show resolved Hide resolved
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_3103/

anishagg17 and others added 2 commits March 20, 2020 22:45
@anishagg17 @elizabetdev
Update src-docs/src/views/code/code_block_pre.js
f8bf65c 
Co-Authored-By: Elizabet Oliveira <elizabet.oliveira@elastic.co>
@anishagg17
updated test
8dd0cbf
@elizabetdev
Copy link
Contributor

jenkins test this

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_3103/

@elizabetdev
Copy link
Contributor

jenkins test this

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_3103/

@elizabetdev
Copy link
Contributor

Hi @anishagg17,

I found one more issue. The src-docs/src/views/code/code_block_pre.js example was missing a whiteSpace="pre". I also improved the text.

Here's the PR: anishagg17#8

@anishagg17
Copy link
Contributor Author

Pr merged 👍

@elizabetdev
Copy link
Contributor

jenkins test this

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_3103/

elizabetdev
elizabetdev approved these changes Mar 23, 2020
View reviewed changes
Copy link
Contributor

@elizabetdev elizabetdev left a comment

Choose a reason for hiding this comment

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

LGTM! 🎉

You just need to merge master and update the tests because the build is failing.

@anishagg17
Copy link
Contributor Author

I have merged master , now I think build would pass

@elizabetdev
Copy link
Contributor

jenkins test this

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_3103/

@snide snide merged commit 1eece3d into elastic:master Mar 23, 2020
@snide
Copy link
Contributor

snide commented Mar 23, 2020

Thanks @anishagg17! Double checked my earlier issues and merged before the CL got out of date.

@anishagg17 anishagg17 deleted the wrap branch March 23, 2020 21:24
@chandlerprall chandlerprall mentioned this pull request Mar 31, 2020
Closed
@chandlerprall chandlerprall mentioned this pull request Apr 8, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cchaos cchaos cchaos left review comments

@elizabetdev elizabetdev elizabetdev approved these changes

@snide snide snide approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

<EuiCodeBlock> option to not wrap lines (white-space: pre)
5 participants
@anishagg17 @kibanamachine @elizabetdev @cchaos @snide