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

Code Snippet enhancements #535

Closed
bsonefeld opened this issue Jan 31, 2018 · 38 comments
Closed

Code Snippet enhancements #535

bsonefeld opened this issue Jan 31, 2018 · 38 comments

Comments

@bsonefeld
Copy link
Contributor

Detailed description

Describe in detail the issue you're having. Is this a feature request (new component, new icon), a bug, or a general issue?

Feature request.

Is this issue related to a specific component?

Code Snippet.

What did you expect to happen? What happened instead? What would you like to see changed?

Feedback from Adam Lankford & Co.

  • Would be nice to fix the scroll bar on the terminal snippet (horizontal scroll, can it be vertical)?
  • Some teams are interested in using the "light" code snippet, but have the sizing be similar to the Terminal snippet. Can we remove the max-height on the light code block?

What offering/product do you work on? Any pressing ship or release dates we should be aware of?

Containers / Kubernetes.

@tw15egan
Copy link
Member

@tay-aitken what have we decided to change with our current snippet?

@tay-aitken
Copy link
Contributor

Yep, its on my current sprint

@bsonefeld
Copy link
Contributor Author

I think in general, our thoughts were:

  • Create a "light" version of the terminal code snippet
  • Add more flexibility to the current Code Snippet, so teams are able to change the height

@tay-aitken
Copy link
Contributor

Research

StackOverflow
screen shot 2018-03-05 at 8 41 52 am

Bootstrap
screen shot 2018-03-05 at 8 35 48 am

Medium
screen shot 2018-03-05 at 12 37 54 pm

Code Sandbox
screen shot 2018-03-05 at 12 39 02 pm
screen shot 2018-03-05 at 12 39 06 pm

Clarity
screen shot 2018-03-05 at 12 47 52 pm

Heroku
screen shot 2018-03-05 at 12 41 45 pm

Google Cloud Platform
screen shot 2018-03-05 at 12 43 49 pm

@tay-aitken
Copy link
Contributor

Design :: Round 1

The 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

  • width: 600px;
  • max-height: 650px;
  • min-height: 56px;
  • If content extends past this height a scrollbar is enacted

proposed design

Option 2 Still support both a code and terminal snippet but both snippets use field-01 as the background color. Code snippet has a much more flexible height and and terminal snippet reminds at one line only or a height:56px

Thoughts @aagonzales @bsonefeld @tw15egan?

@bsonefeld
Copy link
Contributor Author

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.

@tw15egan
Copy link
Member

tw15egan commented Mar 6, 2018

Agree with @bsonefeld, if we can have 1 component do it all thats my vote

@tay-aitken
Copy link
Contributor

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?

@tw15egan
Copy link
Member

tw15egan commented Mar 6, 2018

@tay-aitken that should be good. @carbon-design-system/ui how should we go about actually implementing this, without breaking existing users?

@joshblack
Copy link
Contributor

@tw15egan would it follow our pattern of:

  • Release the next version of the component with a v2 suffix
  • Add deprecation notice for old component
  • Swap v2 for classic in breaking change

?

@bsonefeld
Copy link
Contributor Author

I'd be open to exploring more patterns for releasing, if y'all have suggestions 😊

Sometimes I worry the v2 convention confuses people. Thoughts?

@joshblack
Copy link
Contributor

joshblack commented Mar 6, 2018

Definitely down for things other than v2. A common convention that we could borrow from package releases is to use next, but definitely not tied to it 👍

@asudoh
Copy link
Contributor

asudoh commented Mar 7, 2018

Just wondering - Probably our deprecation notice stuff we started using for addons would help v2 confusion?

@alisonjoseph
Copy link
Member

I like next better than v2, but is that even more confusing for our users since we are already using v2 on other components?

@tay-aitken
Copy link
Contributor

Question—other than the terminal snippet deprecation what is the breaking change to the code snippet? Is it the change in height?

@alisonjoseph
Copy link
Member

Good point, if its just a change in height, we might just want to wait until v9 to release the update, do we even need a v2 or next version in the interim?

@joshblack
Copy link
Contributor

joshblack commented Mar 7, 2018

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.

@aagonzales
Copy link
Member

aagonzales commented Mar 7, 2018

Sorry late to this issue, love the research but I have a couple of design questions/concerns.

  • Why are we setting a fixed width (600)? I think we should set a max and min width. I think it will need to be fluid/responsive for smaller screens.
    • I'm also not super convinced we should set an absolute max width. The reason we do it on things like text fields etc is for line length readability but that's not always relevant with code. Sometimes, a code line never breaks and if there is room to take up more than 600px why not let them (if it works with the layout)?
  • Also why is there also set a max height? I don't like this pattern as much. I get its trying to constrain the view port but I think it should work like the code snippets on our website code pages. Where there is a set height that will trigger a scroll but the user also has the option to fully expand the code snippet so they don't have to scroll. If we go with this i think the first max height should actually be smaller than 600 so you can more easily scan the content on a page.
  • Is there a border on the snippet? What does this look like if its on the $ui-02 background?

@tay-aitken
Copy link
Contributor

Why are we setting a fixed width (600)? I think we should set a max and min width. I think it will need to be fluid/responsive for smaller screens.

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.

Also why is there also set a max height?

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.

Is there a border on the snippet? What does this look like if its on the $ui-02 background?
No border, I was trying to change the color styling as little as possible from what we had before since this wasn't meant to be a complete component redesign. I know we had an issue with the field-01 on ui-05 but since this isn't a text input and the user can't click into to area and edit it I didn't feel it needed a field-02 equivalent. If you feel that we need more exploration into that I can do a few comps.

@aagonzales
Copy link
Member

I know we had an issue with the field-01 on ui-05 but since this isn't a text input and the user can't click into to area and edit it I didn't feel it needed a field-02 equivalent.

So a code snippet can only live on a white background?

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.

Agree it should depend on layout but I think there will be use case where it will need to be larger. On our own code tabs for instance. Though maybe that's not a "snippet" maybe this is a second variation that is a code block or something that is meant to fill more space than a "snippet".

Our current snippet is very limiting to the amount of code you can see.

Agreed. Which is why I think should open it up even more by adding in the "view more code" function.
mar-08-2018 14-55-57

@tay-aitken
Copy link
Contributor

Designs :: Round 2

Created 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 1

The 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.
round 2 proposed design 1

Option 2

The 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.
round 2 proposed design 2

Line Numbers

I 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.
round 2 proposed design line numbers

Questions

Do we still want to have a component for one line of code? the 56px tall snippet proposed in the comment above

@bsonefeld
Copy link
Contributor Author

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.

Do we still want to have a component for one line of code?

Yes I think so, since this is the primary use case we've been hearing from teams.

@tay-aitken
Copy link
Contributor

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)

@aagonzales
Copy link
Member

aagonzales commented Apr 4, 2018

Feedback

  • I like option 2. Option 1 seems strange to expand the content and then also still have to scroll within a confined space. Option 2 expansion would be controlled by the normal page scrolling and not internal scrolling like option 1.

  • Question: does option 2 include internal scrolling before expansion or is the only way to see the full set is to click on expand?

    • I kind of just want them to work like the code snippets on our code tab. That may just be my bias though because that's what I've grown used to.
  • Please note that these examples do not allow for horizontal scrolling.

    • Just these examples or code snippets are never allowed to have horizontal scroll?
      Concern: aren't some code lines unbreakable? They just run on in a single line (or that's what i've previously been told, could be wrong)
  • I like adding in the number line option

  • Yes, I still think a single line snippet is still needed

  • What about also adding an inline code style? Here is an example of inline code styling.

@tay-aitken
Copy link
Contributor

tay-aitken commented Apr 4, 2018

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?

screen shot 2018-04-04 at 2 48 49 pm

@aagonzales
Copy link
Member

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.

@tay-aitken
Copy link
Contributor

Did you ever propose a fix for it @aagonzales?

@aagonzales
Copy link
Member

I think it as just making the height of the highlight smaller. It would not be changing the line height.

@tay-aitken
Copy link
Contributor

Inline code :: Round 1

Whatever 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 #F4F7FB, and have padding: 4px on the left and right sides.

Property Option 1 Option 2 Option 3 Option 4
font-size 14 14 14 12
height 26 26 22 22
border-radius 3 4 4 3

inline code

@aagonzales
Copy link
Member

aagonzales commented Apr 5, 2018

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.

@tay-aitken
Copy link
Contributor

tay-aitken commented Apr 5, 2018

Hello there. Mjd test kakdfjakldfaksdfjlkad

@tay-aitken
Copy link
Contributor

Dev Questions

  • Could we do a percentage for the text size since there are different text sizes used across the platform?
  • What are the best practices for a code snippet?
    • is it bad to break a line? (worried about having both horizontal and vertical scrolling)
  • Which of the options above do you prefer?

@tay-aitken
Copy link
Contributor

@carbon-design-system/ui any thoughts on the questions posted above?

@asudoh
Copy link
Contributor

asudoh commented Apr 10, 2018

Just putting my vote:

  • Text size: I think the text size of code and the text size of the rest of the document is a different animal, so I'd prefer not being affected by the font sizes of others
  • Scrolling: Code editor tends to have horizontal scrolling for long lines (While some editors let you switch it back and forth)
  • Option: I'd choose option 2 (no max-height when expanded)

@joshblack
Copy link
Contributor

Could we do a percentage for the text size since there are different text sizes used across the platform?

Percentage is definitely possible, and it would be relative to the parent element's font size 👍

What are the best practices for a code snippet?

  • is it bad to break a line? (worried about having both horizontal and vertical scrolling)

Honestly the only two requirements I look for are:

  1. Easy to copy
  2. Has syntax highlighting for the language I'm viewing

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):

screen shot 2018-04-10 at 3 29 58 pm

Which of the options above do you prefer?

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.

@tay-aitken
Copy link
Contributor

Final Designs

The final designs for the code snippet revamp include:

  • Terminal Snippet (single line)
  • Code Snippet (multi-line)
  • Inline code

All of the following designs should use Plex Mono

Terminal Snippet

Functions the same as the previous terminal snippet (can only scroll horizontally), just getting a visual update.

height:56px;
width: 600px
border: solid 1px #DFE3E6 (ui-03)
background-color: #fff (ui-01)

terminal-snippet

Code Snippet

  • Once the user clicks the 'show more' ghost button the hight of the code snippet expands to show all content contained within the code snippet
  • Can only scroll horizontally, vertical scroll is not needed since component expands to full content size
  • Added a 'show more' and 'show less' button
  • visual updates, color and sizing

height:56px;
width: 250px
border: solid 1px #DFE3E6 (ui-03)
background-color: #fff (ui-01)
code-snippet

Inline Code (new)

  • the text size for the snippet will vary based off the body copy size it is in. If this is not possible let me know and we can revaluate.

height: 22px;
width: varies based on content
background-color: F4F7FB;
corner-radius: 4px;
padding: 0 4px;

inline-code

Important Notes

  • Line numbers and syntax highlighting
    • will be in the documentation and will not be included in the actual code of the component
    • will point to open source versions for numbers and highlighting
    • CodePen will be used as an example to show how the numbers and highlighting will be applied; will be linked in the documentation
    • line numbers will look like this

line-numbers

  • In a future, for next breaking change release, we want to change the names of the components as follows:
    • terminal snippet > single line snippet
    • code snippet > multi-line snippet

@tay-aitken
Copy link
Contributor

@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.

@marijohannessen
Copy link
Contributor

Closing this as it's done 🎉

joshblack pushed a commit to joshblack/carbon that referenced this issue May 2, 2019
…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
joshblack pushed a commit to joshblack/carbon that referenced this issue May 2, 2019
* fix(package): update flatpickr to version 4.2.4

* chore(package): update lockfile

https://npm.im/greenkeeper-lockfile
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

No branches or pull requests

8 participants