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): set closed and expanded max number of rows #8085

Conversation

guigueb
Copy link
Contributor

@guigueb guigueb commented Mar 13, 2021

defaulted the height to show everything (100%)
added two new props maxClosedNumberOfRows and maxExpandedNumberOfRows
added logic using the new props to determine when to show moreLessBtn
added maxHeight style based on new props

Closes #

#7579

Testing / Reviewing

See 7579 for the tests to verify this PR.

Tested:

MacOS - Chrome, FireFox, and Safari
Win10 - Chrome, FireFox, and Edge

The margin fix described in 7582 was not done based on comments in 7826.

Any issues with (scrollbar, WCAG 1.4.10 compliance, minHeight, and overFlow indicators) have been discussed in 7826 will be addressed in these future PRs.
8056 CodeSnippet: there should be a way to set the min number of rows when in closed and expanded mode
7574 CodeSnippet: missing horizontal scrollbar when type is multi and there is a long row of data
8057 CodeSnippet: the scrollable content needs to be WCAG 1.4.10 compliant
8059 CodeSnippet: Remove or improve the overflow indicators

defaulted the height to show everything (100%)
added two new props maxClosedNumberOfRows and maxExpandedNumberOfRows
added logic using the new props to determine when to show moreLessBtn
added maxHeight style based on new props
@guigueb guigueb requested review from a team as code owners March 13, 2021 23:04
@netlify
Copy link

netlify bot commented Mar 13, 2021

Deploy preview for carbon-elements ready!

Built with commit 1735893

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

@netlify
Copy link

netlify bot commented Mar 13, 2021

Deploy preview for carbon-components-react ready!

Built without sensitive environment variables with commit 1735893

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

@guigueb
Copy link
Contributor Author

guigueb commented Mar 14, 2021

Where can I see the conformance list for the platforms and browsers Carbon supports?
I likely missed it reading the Carbon Design System pages.

The reason being... I viewed CodeSnippet on my Samsung Tablet and the number of lines displayed does not match the max values given.
Although, if this is a problem its beyond this PR as it wasn't displaying 15 before this PR.

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.

Carbon targets evergreen browsers, but that would include the mobile variants so I think the issue with the incorrect number of lines being shown on mobile browsers should be addressed together

@guigueb
Copy link
Contributor Author

guigueb commented Mar 16, 2021

@emyarod
I just retested and I am not able to reproduce the issue.
I believe the pixel count per row was off, and when I adjusted rowHeightInPixels from 18 to 16 it addressed the issue.

You mention evergreen browsers... can you point me to the conformance text on the Carbon Design System pages that mentions this?

The list of evergreen browsers (I learnt something) is huge and my ability to test them is limited (I can do mac, win, Samsung Tab S2, and a very old chromebook) so hard to tell if it works everywhere.

@guigueb guigueb requested a review from emyarod March 17, 2021 12:46
@emyarod
Copy link
Member

emyarod commented Mar 17, 2021

@guigueb here is the Carbon browserslist

@emyarod emyarod requested review from a team and kingtraceyj and removed request for a team March 17, 2021 18:18
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.

also requesting ux/visual review for this feature

packages/react/src/components/CodeSnippet/CodeSnippet.js Outdated Show resolved Hide resolved
@tw15egan
Copy link
Member

Since this PR introduces the ability to set the max number of rows when expanded to less than the total number of lines, I think we'll also need to add a selector that allows vertical scrolling, something like

  .#{$prefix}--snippet--multi.#{$prefix}--snippet--expand
    .#{$prefix}--snippet-container {
    overflow-y: auto;
  }

@guigueb
Copy link
Contributor Author

guigueb commented Mar 18, 2021

I completely agree that overflow(scrollbars) should be taken into account for this.
That was attempted in #7826 but scrollbars are not directly related to adding the max props (although related/exacerbated, scrolling is an existing issue with large 100% height CodeSnippets - which is what I'm working on fixing) and introduces issues with WCAG 1.4.10 compliance.
As such they should be dealt with in #7574 and #8057.

@guigueb guigueb requested a review from emyarod March 18, 2021 11:57
@tw15egan
Copy link
Member

tw15egan commented Mar 18, 2021

@guigueb in the current implementation, when the content is expanded, it shows every line of code. If this PR introduces the ability to show less than the max number, you need to be able to scroll to the bottom to view all the code, and should not be handled in a separate PR.

Example: In the preview, set the max expanded to 16. When you hit expand, it only expands 1 line, and the rest of the code cannot be scrolled to

@guigueb
Copy link
Contributor Author

guigueb commented Mar 18, 2021

I aware of the issue and agree, I tried to address this with my other PR - and was told to stick to the props.
I'll add changes for overflow back into this one, keep in mind it will be non-WCAG 1.4.10 compliant.

With current CodeSnippet showing 100% if there are 20000 lines you see only what you can... and the rest of the code cannot be scrolled to (same issue different number of max rows).
I suppose you can work around this by putting it in a scrollable container but that only causes other issues.
The vertical scroll issue isn't new but it is easer to see, with the limit.

@tw15egan
Copy link
Member

tw15egan commented Mar 18, 2021

@guigueb it seems like we just need to add the line I suggested to fix that issue. The current implementation will push all other content down, no matter the number of lines. I'm not seeing any "unreachable" code

scroll

@emyarod emyarod requested review from a team and aagonzales and removed request for kingtraceyj and a team March 19, 2021 14:24
Bill Guigue added 3 commits March 22, 2021 09:24
allow snippet-mult + snippet-container to auto scroll, both x and y
remove scroll/overflow from nested snippet-expand, pre, and code conditions
this will show the horizontal scroll when the snippet is closed or expanded
it is a behavour change/bug fix, it will address 7574
add background to buttons to stop scrollbars from stomping on them
changing to overflow - from -x/-y - on the snippet-container
caused issues with scrollbars colliding with the buttons.
for now only add overflow-y and address overflow clean up and
scrollbars/button collisions in carbon-design-system#7574
apply it for both closed and expanded modes
@guigueb guigueb requested a review from tw15egan March 22, 2021 19:38
Copy link
Member

@tw15egan tw15egan 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 working on this! LGTM 👍 ✅

Copy link
Member

@tay1orjones tay1orjones left a comment

Choose a reason for hiding this comment

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

Just one small piece of feedback on prop naming. Looks great otherwise!

packages/react/src/components/CodeSnippet/CodeSnippet.js Outdated Show resolved Hide resolved
Bill Guigue added 2 commits March 23, 2021 11:09
Copy link
Member

@aagonzales aagonzales left a comment

Choose a reason for hiding this comment

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

Looks like it's working and looking correct to me!

@kodiakhq kodiakhq bot merged commit cfcda43 into carbon-design-system:main Mar 23, 2021
@guigueb guigueb deleted the feat-(codesnippet)-set-closed-and-expanded-max-number-of-rows branch March 24, 2021 15:30
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.

5 participants