-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Code Snippet enhancements #535
Comments
@tay-aitken what have we decided to change with our current snippet? |
Yep, its on my current sprint |
I think in general, our thoughts were:
|
Design :: Round 1The research above showed me that most products that use a code and terminal snippet do not differentiate between the two and just use one component for all use cases. Based on feedback that we have gotten about the terminal snippet being too dark and standing out too much on the page, I think we have two options. Option 1 Deprecate the terminal snippet in our next release and make the proposed changes to the code snippet, recommending that the code snippet be used for all code (and terminal) scenarios
Option 2 Still support both a code and terminal snippet but both snippets use Thoughts @aagonzales @bsonefeld @tw15egan? |
I prefer Option 1. I don't think it makes sense to have two variations of the Snippet component if we can add flexibility to one option. |
Agree with @bsonefeld, if we can have 1 component do it all thats my vote |
Great I was leaning towards that option as well. Then let's move this to development. @tw15egan do you need more specs then what I have above? |
@tay-aitken that should be good. @carbon-design-system/ui how should we go about actually implementing this, without breaking existing users? |
@tw15egan would it follow our pattern of:
? |
I'd be open to exploring more patterns for releasing, if y'all have suggestions 😊 Sometimes I worry the |
Definitely down for things other than |
Just wondering - Probably our deprecation notice stuff we started using for addons would help |
I like |
Question—other than the terminal snippet deprecation what is the breaking change to the code snippet? Is it the change in height? |
Good point, if its just a change in height, we might just want to wait until |
I think what @tw15egan was calling out yesterday is that we have the terminal classes baked into the snippet, versus just having a terminal module, which makes deprecation inside of the module more difficult. The breaking change here would most likely be just the removal of the terminal modifier class names. When it comes to releases, I think we can introduce the changes under the current selectors in a minor release (like @alisonjoseph pointed out), as long as they don't break existing usage of code snippets. One thing we could do is, in the next major change, move over the terminal stuff into it's own module and add the deprecation warnings. When people go to update, they can move stuff over as-is and keep using the terminal stuff until the next release, or just invest the same amount of work and refer to the new code snippet as part of the upgrade. |
Sorry late to this issue, love the research but I have a couple of design questions/concerns.
|
I fine with switching it to a max-width and min-width. I set it to a max of 600px since in most of the cases where I say code and terminal snippets in product they are with a lot of text. I preferred they be in line with the width of the text as opposed to jutting out past the content.
I set a larger height based off of feedback that the current snippet wasn't showing enough and that most other products that have a code snippet show a lot more code than we did (see above examples). I figured we should show more code than less. Our current snippet is very limiting to the amount of code you can see.
|
Designs :: Round 2Created some new iterations based off of Anna's comments, using the 'Show more' ghost button. Please note that these examples do not allow for horizontal scrolling. Option 1The code snippet is always starts at a height of 288px. Once the user clicks 'Show more' button the code snippet expands up to a height of 650px. If more space is needed for content a scroll bar will be enacted. Option 2The code snippet is always starts at a height of 288px. Once the user clicks 'Show more' button the code snippet expands up to a height of the rest of the content. In this instance a 'Show less' button would appear since these snippets could be very long. Line NumbersI also think we should have line numbers offered as a modifier for situations (for example when there is an excessively long code snippet) that need it. QuestionsDo we still want to have a component for one line of code? the 56px tall snippet proposed in the comment above |
I like Option 2. I think if we show them the entire Code sample it could be really overwhelming, so I like having the additional option to show/hide. Line numbers - this is a nice addition! What size are the numbers, 14? I think they could be a tad smaller.
Yes I think so, since this is the primary use case we've been hearing from teams. |
Just clarifying option 1 also has show more but expands to a max-height (with scroll bar if necessary) option two also uses show more and expands to show the entire code snippet. The line numbers are the same size as the code for text alignment purposes (so the baselines align) |
Feedback
|
I mean if its easier so we don't keep going through design rounds we can use the code snippet we already have and just change the background color. I noticed on CodePen they break lines often, but maybe this is bad practice and something we shouldn't be following. Alright I'm keeping the 56px component as I have it above. Yes I can also do an inline coding style. I like what we have on the carbon site already though I wouldn't mind rounding the borders a bit from the current style. I also noticed that the inline snippets overlap due to certain line heights, I'm assuming this isn't intentional? |
Correct, not intentional. It was just never fixed. |
Did you ever propose a fix for it @aagonzales? |
I think it as just making the height of the highlight smaller. It would not be changing the line height. |
Inline code :: Round 1Whatever variation we settle on here should be adopted for the carbon site as there are currently come overlapping issues with that variation. Below, all examples are using IBM Plex mono, the new field-01 color
|
I like option 3 the best. Though it doesn't bother me as much to have the height be 26 and touching when the background color isn't transparent. I think 12px text feels a little out of place though I get why you tried it. The 14 also seems big. |
Hello there. Mjd |
Dev Questions
|
@carbon-design-system/ui any thoughts on the questions posted above? |
Just putting my vote:
|
Percentage is definitely possible, and it would be relative to the parent element's font size 👍
Honestly the only two requirements I look for are:
For the line break stuff, it's definitely not bad to break a line. Typically editors support this as an option in their editor. If someone has the line wrap option on, then some character on screen represents that the line has wrapped. For example the carriage return character (might have to open it up to view the character):
Option 4 for inline 👍 Really only because the blocks don't touch when the line above or below has a code snippet as well, and visually it seems like the smaller font is more inline than the larger ones. |
@tw15egan is there a place that we have the current syntax-highlighting colors documented that I could take a look at? Just want to make sure they meet color contrast ratios. |
Closing this as it's done 🎉 |
…m#575) * fix(misc): misc fixes (carbon-design-system#530) * chore(lint): remove console.log from overflow * chore(build): remove node 4 from travis * chore(ButtonSet): mark as deprecated (carbon-design-system#510) Official carbon-components no longer need to use explicit containers for pairing button components. Buttons will space themselves when primary and secondary pairs are created. * chore(CopyButton): deprecate the CopyButton docs(CopyButton): mention availability of new CodeSnippet component docs(CopyButton): remove ticks * fix(InnerLeftNav): add missing aria-label Fixes carbon-design-system#535 test(InteriorLeftNavList): update baseline a11y results
* fix(package): update flatpickr to version 4.2.4 * chore(package): update lockfile https://npm.im/greenkeeper-lockfile
Detailed description
Feature request.
Code Snippet.
Feedback from Adam Lankford & Co.
max-height
on the light code block?Containers / Kubernetes.
The text was updated successfully, but these errors were encountered: