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

feat(CodeSnippet): add support for word wrap #6960

Merged

Conversation

echarpibm
Copy link
Contributor

Closes #6473

Adds support for text wrapping on a type multi CodeSnippet

Changelog

New

  • New boolean property wrapText
<CodeSnipper type="multi" wrapText=true>
the code in here
</CodeSnippet>

Changed

  • Nothing

Removed

  • Nothing

Testing / Reviewing

In storybook, the CodeSnippet will show a new story for the Longline as well as the new property in the knobs which can be turned on or off to see the behaviour.

@echarpibm echarpibm requested a review from a team as a code owner October 2, 2020 12:41
@netlify
Copy link

netlify bot commented Oct 2, 2020

Deploy preview for carbon-elements ready!

Built with commit 6c87622

https://deploy-preview-6960--carbon-elements.netlify.app

@netlify
Copy link

netlify bot commented Oct 2, 2020

Deploy preview for carbon-components-react ready!

Built without sensitive environment variables with commit 6c87622

https://deploy-preview-6960--carbon-components-react.netlify.app

@emyarod emyarod requested review from a team and laurenmrice and removed request for a team October 2, 2020 16:30
Copy link
Member

@emyarod emyarod left a comment

Choose a reason for hiding this comment

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

looks good to me, just pending visual review (the text may need to be updated in the storybook example)

also in order to pass CI you will need to update the component snapshots (yarn test -u)

@dakahn
Copy link
Contributor

dakahn commented Oct 3, 2020

this actually may be a needed accessibility enhancement given the coming WCAG requirements around text reflow 👍 🏄

@laurenmrice
Copy link
Member

I think the story longline should be represented as a prop or knob? because it is not a distinct variant like inline, single line and mutli-line code snippet. @emyarod do you agree?

@echarpibm
Copy link
Contributor Author

I think the story longline should be represented as a prop or knob? because it is not a distinct variant like inline, single line and mutli-line code snippet. @emyarod do you agree?

I considered that when I created the story, but essentially the existing stories do not show "long lines" and the actual text is not configurable. So I chose to leave the existing stories as they were and to add one that showed long lines in it. I considered it to be "more documentation" and more clear than to "hide" the behaviour in one of the existing stories.

@emyarod
Copy link
Member

emyarod commented Oct 5, 2020

@laurenmrice @echarpibm I think it's fine to expose it as a storybook knob and just update the existing code snippets to contain long lines of text. My original question was around the actual code snippet example content: do we have something else we would rather use in place of thisisasecondlinethatisverylongbuthasnospacestoseehowitbehavesinthissituationwillitwrapanywaythisiswhatweneedotseeherethisisasecondlinethatisverylongbuthasnospacestoseehowitbehavesinthissituationwillitwrapanywaythisiswhatweneedotseehere for example?

@echarpibm
Copy link
Contributor Author

@laurenmrice @echarpibm I think it's fine to expose it as a storybook knob and just update the existing code snippets to contain long lines of text. My original question was around the actual code snippet example content: do we have something else we would rather use in place of thisisasecondlinethatisverylongbuthasnospacestoseehowitbehavesinthissituationwillitwrapanywaythisiswhatweneedotseeherethisisasecondlinethatisverylongbuthasnospacestoseehowitbehavesinthissituationwillitwrapanywaythisiswhatweneedotseehere for example?

Like this? Or do you want actual code?

Lorem ipsum dolor sit amet, consectetur adipiscing elit. Curabitur varius molestie feugiat. Morbi ut lectus vel urna auctor congue. Vivamus pellentesque pulvinar justo, vitae sodales sapien volutpat sed. Quisque blandit nisi eu erat dapibus posuere. Integer mattis suscipit neque. Phasellus sed pulvinar leo, vitae tristique magna. Donec ex nisl, lobortis nec tincidunt at, suscipit ac risus. Ut vestibulum porta est. Mauris justo justo, blandit id congue id, egestas a felis. Suspendisse condimentum mauris vel rutrum scelerisque. Cras massa leo, interdum at libero at, tincidunt tincidunt nunc.

@laurenmrice
Copy link
Member

I think if you had a real usecase example it would be great , like actual code.

@echarpibm
Copy link
Contributor Author

I think if you had a real use case example it would be great , like actual code.

I removed the extra example and modified the Multiline to be half the width of the screen and update the content of the code snippet with a package.json extract that allows to show the new functionality. Let me know if that works.

Copy link
Member

@laurenmrice laurenmrice left a comment

Choose a reason for hiding this comment

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

Looks good to me! thank you 🙌🏻

Copy link
Member

@emyarod emyarod left a comment

Choose a reason for hiding this comment

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

looks good to me, just need to update snapshots to pass CI now (yarn test -u)

@echarpibm
Copy link
Contributor Author

looks good to me, just need to update snapshots to pass CI now (yarn test -u)

So I'm not sure if that is directed to me or not. But for some reason, I never got the tests to pass fully on my computer (and still don't) so I am not sure what would be expected of me in this case. Sorry.

@echarpibm echarpibm requested a review from a team as a code owner October 6, 2020 21:47
@kodiakhq kodiakhq bot merged commit 5417c4e into carbon-design-system:master Oct 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add text wrapping to Codesnippet
5 participants