fix(CodeSnippet): set number of closed and open rows #7826

guigueb · 2021-02-17T14:56:26Z

remove minHeight/maxHeight from scss
rework overflow settings to allow x and y scrolling
updated stories to allow knobs in all variations
updated knobs to include all the props
updated stories to show props not ...args in docs
added min/max closed/expanded props to CodeSnippet
changed rowHeight from 18 to 16, changing rows showed I was off a little
update shouldShowMoreLessBtn dynamically if data changes
update expandedCode dynamically if data changes
add minHeight/maxHeight styles to snippet-container
add PropTypes for the new props

BREAKING CHANGE: expand is set to 30 rows from unlimited

Closes #

#7579
#7574

Tested:

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

The behavior is good and as expected ( I think ).

But I did noticed one thing...

in browsers on Win10 the scrollbar is solid and always shown
in browsers on MacOS the scrollbar is transparent and only shows when you scroll

This effects two things...

overflow indicators are not needed on windows, scrollbars indicate overflow
a max height (maxClosedNumberOfRows/maxExpandedNumberOfRows) of say 5 will render 5 rows but,...
-- on MacOS you see all 5 rows, when scrolling the last row is covered with a transparent scrollbar
-- on Win10 you will see 5 rows if no horizontal scroll is available, but if so the last row will not be seen as it will always have a solid scroll bar covering it
--- I thought of determining the OS and adjusting when scrollable and not scrollable but this would change the size of the CodeSnippet and lead to layout issues. Best to document this behavior as expected.

Questions/confirmations:

I thank TJ for replying about the spacing of the copy button when in multi-mode. Can UX confirm that is the design we want to stick with?
Who needs to discuss the use/keeping of the overflow indicators? They are only applicable on Mac as other OS's show scroll bars, they are only useful when there is something in the visible shade area to be shaded, and the shading is very subtle and easily overlooked.
Since a MAC user is aware/used to hidden scrollbars and have the option of turning them on (https://www.matuzo.at/blog/css-pro-tip-for-mac-users-always-show-scroll-bars-in-macos/) I think the overflow indicators should be removed - optionally we could always force them on for CodeSnippet.

add closedNumberOfRows and expandedNumberOfRows props set mixin max-height using closedMaxHeight and expandedMaxHeight determine show button visibility using closed/expanded NumberOfRows sort consts and prop

netlify · 2021-02-17T15:06:17Z

Deploy preview for carbon-elements ready!

Built with commit c7ba1bf

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

netlify · 2021-02-17T15:16:11Z

Deploy preview for carbon-components-react ready!

Built without sensitive environment variables with commit c7ba1bf

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

add new CodeSnippet props to Storybook

…umber-of-closed-and-open-rows

guigueb · 2021-02-17T18:39:50Z

Part of the fix will be to pass a variable from the CodeSnippet React class to the the CodeSnippet mixin (the PR has both these working independently).
ie: based on a new CodeSnippet Class prop (closedNumberOfRows) the scss max-height style needs to be set

Anyone have guidance on how to do this or of an existing example where this is being done?
I’m not sure how to pass a variable to the scss files.

Update:
It seems that what I want to do can be accomplished via css custom properties.
And yay, there is a hook for it… https://www.npmjs.com/package/react-use-css-custom-property

So now the questions are...

is this an acceptable thing to do in Carbon?
is there a process to get this, or any, React hook into Carbon?

I rewrote the code to set height via style.

guigueb · 2021-03-02T21:14:13Z

Question about button positions.
The more/less button is right in the corner of the snippet container while the copy button is not.
I think the copy button should be right in the corner as well, and can change that by removing these two lines.

.#{$prefix}--snippet--multi .#{$prefix}--copy-btn {
top: $carbon--spacing-03;
right: $carbon--spacing-03;

tw15egan · 2021-03-02T21:17:08Z

I believe the CopyButton is positioned there as designed

dakahn · 2021-03-05T22:10:01Z

I can't seem to get this to work as I understand it should in Firefox on Windows 10 Clicking show/hide shows or hides rows that don't contain code.

Also worth noting is I can select negative rows in the playground addons panel for this.

remove minHeight/maxHeight from scss rework overflow settings to allow x and y scrolling updated stories to allow knobs in all variations updated knobs to include all the props updated stories to show props not ...args in docs added min/max closed/expanded props to CodeSnippet changed rowHeight from 18 to 16, changing rows showed I was off a little update shouldShowMoreLessBtn dynamically if data changes update expandedCode dynamically if data changes add minHeight/maxHeight styles to snippet-container add PropTypes for the new props BREAKING CHANGE: expand is set to 30 rows from unlimited

…umber-of-closed-and-open-rows

guigueb · 2021-03-05T23:41:40Z

With the latest commit I think I'm able to take the Draft tag off this PR now.
It should be good to review now.

There is still a glitch with the overflow indicators (the shading of characters to indicate scrollable content left and right) but I'm hoping to discuss this and remove the indicators.
They are very subtle and at times useless - maybe we can find a better way to accomplish their task.

@dakahn mentioned that the playground values can be negative.
This is true, and like the prop itself the playground prop is whatever the user wants it to be.
We assume they will give good values, but if not oh well.
Similar to type having to be 'inline', 'single', or 'multi'... it the user passes 'cow' oh well.
I'll read up on StoryBook and see if it can limit the number values.
And I'll check if Storybook also allows it to only be integer values, too.

On this note unexpected things may happen if users try to set the min/max closed/expanded to odd numbers...

closed max = 20, expanded max = 10
Is this something we need to check?
As mentioned, we don't check if user passes type="cow"

updated snapshot added {min=1} governer to Storybook numbers

guigueb · 2021-03-06T18:16:12Z

I tested this on Mac and Win10, and updated the description to show this.

guigueb · 2021-03-06T19:02:16Z

I did some testing on windows.
The scroll bars will always show if scrollable is set, even if the data does not warrant it.

ie: if overflow:scroll is set you will get x and y scrollbars on windows regardless of the data.
I can set overflow-x:hidden when wrapText:true to remove the horizontal scrollbar.
But if the user set wrapText:false the scrollbar will show regardless of the data.

On platforms when the scrollbars always shows, this change will remove the horizontal srollbar when wrapText=true as we will never need it

…umber-of-closed-and-open-rows

Alphabetical - moved max above min

moved width before overflow set Story defaults to match CodeSnippet defaults

…umber-of-closed-and-open-rows

updated CodeSnippet-story copyright year added missed component- in the id when merging code

dakahn · 2021-03-08T22:40:40Z

The negative numbers aren't a blocker, just worth mentioning in case someone with some Storybook experience knew a quick fix.

dakahn

Tested NVDA with Firefox and JAWS with Edge/Chrome on Windows 10
A couple of notes:

In JAWS latest and NVDA the Show More button's label is being read twice as "Show more, show more button", could be some unneeded aria causing this.
In Edge/Chrome the scrolling content in not keyboard accessible
the horizontal scrolling is a violation of WCAG 1.4.10. The code snippet should reflow down the page and not require scrolling in two dimensions.

tw15egan · 2021-03-08T23:49:48Z

packages/react/src/components/CodeSnippet/CodeSnippet-story.js

-  hideCopyButton: boolean('Hide copy button (hideCopyButton)', false),
+const typeOptions = {
+  inline: { inline: 'inline' },
+  single: { single: 'single' },


Is there any reason why we're changing all these storybook lines? This is adding quite a bit of bloat to the PR

Most - not all - of the props were defined in the const props.
Deconstructing the const props in a story shows the props it contains in the stories knobs.
The stories code was then limited - not showing the props used to create it but props deconstructed {...props} and the stories knobs shows props unrelated to the variant it was describing.

To make the Stories more clear I wanted to show the specific props used by each variant.
Initially I created four (inline/single/multi/skeleton) consts but found there was a lot of duplicate code.
So I created one const for each prop and have each variant deconstruct the props uses.
This - in my view - is cleaner and will allow for further Stories to be easily created.

I'm ok with going back to one uber const that contains all props.
But if we do, I would suggest we have only one Story(Playground) that uses it.
Have the other Stories code the exact props it uses, so the users can see what is being used/set to create the Story.

guigueb · 2021-03-09T13:06:05Z

packages/react/src/components/CodeSnippet/CodeSnippet-story.js

+  maxClosedNumberOfRows: number(
+    'maxClosedNumberOfRows: Specify the maximum number of rows to be shown when in closed view',
+    15,
+    { min: 1 }


I added this as a comment on the code so any discussion about it can be kept in a thread.

From @dakahn

It is worth noting is I can select negative rows in the playground addons panel for this.
The negative numbers aren't a blocker, just worth mentioning in case someone with some Storybook experience knew a quick fix.

On the one hand we may want Storybook to be user friendly and only allow what the user should enter.
Towards this I added 'min' entry, and would like to add 'scale' and maybe coded limiters...

On the other hand we may want Storybook to accept anything the user may pass to the Component.
This way we can dup problems, and/or test that bad values are handled properly.

I see both sides, I'm torn (but lean towards leaving it open), but will do whatever Carbon wants.

guigueb · 2021-03-09T13:08:00Z

packages/react/src/components/CodeSnippet/CodeSnippet.js

+  showLessText = defaultShowLessText,
+  showMoreText = defaultShowMoreText,
+  type = defaultType,
+  wrapText = defaultWrapText,


I added this as a comment on the code so any discussion about it can be kept in a thread.

From @dakahn

Tested NVDA with Firefox and JAWS with Edge/Chrome on Windows 10
A couple of notes:

In JAWS latest and NVDA the Show More button's label is being read twice as "Show more, show more button", could be some unneeded aria causing this.
In Edge/Chrome the scrolling content in not keyboard accessible
the horizontal scrolling is a violation of WCAG 1.4.10. The code snippet should reflow down the page and not require scrolling in two dimensions.

The first two issues were not introduced (they existed before).
I didn't notice them and will try to fix them, if this PR doesn't get scrapped because of WCAG 1.4.10.

Humm, I have now did a little reading on WCAG 1.4.10 and have a little knowledge (so I'm a danger to myself and others).

If CodeSnippet is to be WCAG 1.4.10 compliant and not be except by falling into the category of 'content which requires two-dimensional layout for usage or meaning cannot reflow without losing meaning' then I think we have more to discuss.

Things like...

the wrapText prop should be deprecated and wrapText is always TRUE

wrapText can not simply be a break on whitespace, it will need to understand content breaks

we need props to help us understand what we are displaying to determine how to display it
-- we need to know if its a horizontal or vertical locale, to determine which way to scroll
-- we need to know if its a LTR or RTL locale, to determine if we start left or right justified

there may be more

What is the next step to discussing this?
Who do we need to get involved?

The first issue was previously present. I'll open a ticket to track.

The second issue -- scrolling is not keyboard accessible -- isn't present for me in prod because I have no scrollbars whatsoever when code is hidden (which is another problem altogether).

There could be something i'm misunderstanding here, but if your feature introduces scrollbars on Windows they need to be keyboard accessible.

To be 1.4.10 compliant we need to maintain the text reflow we currently have in prod seen here

as for things like wrapText -- as long as the out-of-the-box experience meets WCAG requirements we can document the problems with props that cause violations and we're good as far as v10 is concerned. We can look into deprecating wrapText in future versions though for sure.

You are right, when hidden there is no scrollbar - but you can still scroll - which is one of the problems I was trying to address.
You can, In prod, end up with two way scrolling; if wrapText=false and the CodeSnippet has enough rows you do get two scrollbars.
One is on the CodeSnippet and one is on the container the snippet is in.
I made the issue much easier to encounter by enabling vertical scrolling and always showing the horizontal scrolling.

The existing issues, and my changes effect any OS, not just windows.

double.scrolling.mov

Don't get me wrong, I understand my solution is not 1.4.10 compliant.
I just think to be 1.4.10 compliant more discussion is needed on the points I made earlier.

Actually, as the issue is simply an issue with two scrollbars.
So isn't the solution - at least for horizontal LTR languages to disallow wrapText?
This would allow only the vertical scrollbar and therefor be 1.4.10 compliant.

We still have to deal with...

horizontal RTL latin languages - which should start right justified

vertical languages - which should only have a vertical scroll bar

and a proper wrapText to ensure we are breaking in correct places

jnm2377 · 2021-03-09T17:18:13Z

@guigueb thanks for contributing this. It seems like there are some design changes here that weren't discussed in the original issue and might be out of scope for this PR. I'm mainly referring to some of the padding/spacing changes made, which as you noted, hide some of the code snippet content even when expanded. This is why we have the padding on there.

jnm2377 · 2021-03-09T17:22:15Z

@guigueb I think this PR should just focus on adding the min/max row functionality (#7579). The rest of the style changes that you proposed still need to be reviewed by our designers in the original issue (#7574).

jnm2377

Please keep the scope of the PR to just the min/max row functionality, no other design changes until they've been approved by our designers in the issue.

guigueb · 2021-03-09T20:19:15Z

@jnm2377

I think I only made one padding related change... which I was told to do by @laurenmrice in PR7582.
I did ask about padding around the copy button but @tw15egan said it was by design so I did not change it.

Fixing the scrolling functionality is needed, currently CodeSnippet is not useable with large data.
The max/min props will help with large data but without a scrolling solution CodeSnippet is not useable.
As @dakahn has mentioned the scrollbar change I made is not 1.4.10 compliant.

At this point I think we either need to flush out 1.4.10 compliant-ness now or live with this and address 1.4.10 compliant later.

I may be wrong but I don't think I changed anything else.
I am very willing to discuss this, maybe via webex, to make sure we all are on the same page, with both the problems and what needs to be done.

To be clear:
I know this change is not a final solution because it is not 1.4.10 compliant.
But it will make CodeSnippet useable with large data, which it currently is not.

tw15egan · 2021-03-09T20:28:18Z

@guigueb I think Josefina is just pointing out that this may need to break into several smaller PR's, with a defined scope for each. I would not expect a PR adding in a change for the number of rows shown when opened/closed to contain almost 500 line changes.

I think to achieve this functionality we need to:

Add two new props, closedNumberOfRows and expandedNumberOfRows, that default to the current behavior
Calculate the height in CodeSnippet.js and apply an inline style where needed
- Additional logic to ensure max > min, etc.. and return the defaults if that isn't the case
Add two knobs to the Playground story to let users try different values

Basically, the existing user experience should not change for an end-user unless they opt into adding the two new props.

guigueb · 2021-03-10T13:29:23Z

Looks like I closed this PR.. it was not my intention, I must have hit the wrong button.
Maybe that was for the better anyway as the fixes this PR delivers need to be broken into several smaller deliveries.

I have created smaller enhancement issues, listed below, that this PR can be broken up to address.
(#7574 will likely be addressed by #8057)

#7579 CodeSnippet: there should be a way to set the number of rows when in closed and expanded mode
#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

guigueb · 2021-03-10T13:35:02Z

Side note: about 400 lines of this change are to make the stories more intractable and explanatory to the user.
Dropping the updates to stories will bring this down to around 100 lines of code.

guigueb · 2021-03-10T18:51:11Z

I also logged #8059 to address the overflow indicator issue.

fix(CodeSnippet): set number of closed and open rows

c030098

add closedNumberOfRows and expandedNumberOfRows props set mixin max-height using closedMaxHeight and expandedMaxHeight determine show button visibility using closed/expanded NumberOfRows sort consts and prop

guigueb requested a review from a team as a code owner February 17, 2021 14:56

guigueb requested review from tay1orjones and dakahn February 17, 2021 14:56

guigueb marked this pull request as draft February 17, 2021 14:56

Bill Guigue added 2 commits February 17, 2021 11:00

fix(CodeSnippet): add new CodeSnippet props

9531874

add new CodeSnippet props to Storybook

Merge remote-tracking branch 'upstream/master' into CodeSnippet-set-n…

8d45e6d

…umber-of-closed-and-open-rows

Bill Guigue added 2 commits March 5, 2021 17:40

Merge remote-tracking branch 'upstream/master' into CodeSnippet-set-n…

22821ff

…umber-of-closed-and-open-rows

guigueb marked this pull request as ready for review March 5, 2021 23:43

feat(CodeSnippet) fix tests and Storybook

e5ab93e

updated snapshot added {min=1} governer to Storybook numbers

guigueb requested a review from a team as a code owner March 6, 2021 00:07

feat(CodeSnippet) add Storybook govener

08e7aca

Bill Guigue added 5 commits March 8, 2021 09:29

feat (CodeSnippet) always hide scrollbar when wrapText=true

54202a2

On platforms when the scrollbars always shows, this change will remove the horizontal srollbar when wrapText=true as we will never need it

Merge remote-tracking branch 'upstream/master' into CodeSnippet-set-n…

26d6c77

…umber-of-closed-and-open-rows

feat (CodeSnippet) merged to head

b7d0ff0

Alphabetical - moved max above min

feat (CodeSnippet) fixing css prop order

053224c

feat (CodeSnippet) fixed styling and default issues

c7ba1bf

moved width before overflow set Story defaults to match CodeSnippet defaults

Base automatically changed from master to main March 8, 2021 16:35

Bill Guigue added 2 commits March 8, 2021 11:39

Merge remote-tracking branch 'upstream/master' into CodeSnippet-set-n…

9dd1d1a

…umber-of-closed-and-open-rows

feat (CodeSnippet) address format:diff issues

0b971d9

feat (CodeSnippet) missed mdx merge

919b552

updated CodeSnippet-story copyright year added missed component- in the id when merging code

dakahn suggested changes Mar 8, 2021

View reviewed changes

tw15egan reviewed Mar 8, 2021

View reviewed changes

guigueb commented Mar 9, 2021

View reviewed changes

jnm2377 suggested changes Mar 9, 2021

View reviewed changes

guigueb closed this Mar 9, 2021

guigueb mentioned this pull request Mar 13, 2021

feat(CodeSnippet): set closed and expanded max number of rows #8085

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(CodeSnippet): set number of closed and open rows #7826

fix(CodeSnippet): set number of closed and open rows #7826

guigueb commented Feb 17, 2021 •

edited

Loading

netlify bot commented Feb 17, 2021 •

edited

Loading

netlify bot commented Feb 17, 2021 •

edited

Loading

guigueb commented Feb 17, 2021 •

edited

Loading

guigueb commented Mar 2, 2021

tw15egan commented Mar 2, 2021

dakahn commented Mar 5, 2021

guigueb commented Mar 5, 2021 •

edited

Loading

guigueb commented Mar 6, 2021

guigueb commented Mar 6, 2021

dakahn commented Mar 8, 2021

dakahn left a comment

tw15egan Mar 8, 2021

guigueb Mar 9, 2021 •

edited

Loading

guigueb Mar 9, 2021 •

edited

Loading

guigueb Mar 9, 2021 •

edited

Loading

guigueb Mar 9, 2021 •

edited

Loading

guigueb Mar 9, 2021

dakahn Mar 9, 2021 •

edited

Loading

dakahn Mar 9, 2021

guigueb Mar 9, 2021

guigueb Mar 9, 2021

jnm2377 commented Mar 9, 2021

jnm2377 commented Mar 9, 2021

jnm2377 left a comment

guigueb commented Mar 9, 2021 •

edited

Loading

tw15egan commented Mar 9, 2021

guigueb commented Mar 10, 2021 •

edited

Loading

guigueb commented Mar 10, 2021 •

edited

Loading

guigueb commented Mar 10, 2021

fix(CodeSnippet): set number of closed and open rows #7826

fix(CodeSnippet): set number of closed and open rows #7826

Conversation

guigueb commented Feb 17, 2021 • edited Loading

netlify bot commented Feb 17, 2021 • edited Loading

netlify bot commented Feb 17, 2021 • edited Loading

guigueb commented Feb 17, 2021 • edited Loading

guigueb commented Mar 2, 2021

tw15egan commented Mar 2, 2021

dakahn commented Mar 5, 2021

guigueb commented Mar 5, 2021 • edited Loading

guigueb commented Mar 6, 2021

guigueb commented Mar 6, 2021

dakahn commented Mar 8, 2021

dakahn left a comment

Choose a reason for hiding this comment

tw15egan Mar 8, 2021

Choose a reason for hiding this comment

guigueb Mar 9, 2021 • edited Loading

Choose a reason for hiding this comment

guigueb Mar 9, 2021 • edited Loading

Choose a reason for hiding this comment

guigueb Mar 9, 2021 • edited Loading

Choose a reason for hiding this comment

guigueb Mar 9, 2021 • edited Loading

Choose a reason for hiding this comment

guigueb Mar 9, 2021

Choose a reason for hiding this comment

dakahn Mar 9, 2021 • edited Loading

Choose a reason for hiding this comment

dakahn Mar 9, 2021

Choose a reason for hiding this comment

guigueb Mar 9, 2021

Choose a reason for hiding this comment

guigueb Mar 9, 2021

Choose a reason for hiding this comment

jnm2377 commented Mar 9, 2021

jnm2377 commented Mar 9, 2021

jnm2377 left a comment

Choose a reason for hiding this comment

guigueb commented Mar 9, 2021 • edited Loading

tw15egan commented Mar 9, 2021

guigueb commented Mar 10, 2021 • edited Loading

guigueb commented Mar 10, 2021 • edited Loading

guigueb commented Mar 10, 2021

guigueb commented Feb 17, 2021 •

edited

Loading

netlify bot commented Feb 17, 2021 •

edited

Loading

netlify bot commented Feb 17, 2021 •

edited

Loading

guigueb commented Feb 17, 2021 •

edited

Loading

guigueb commented Mar 5, 2021 •

edited

Loading

guigueb Mar 9, 2021 •

edited

Loading

guigueb Mar 9, 2021 •

edited

Loading

guigueb Mar 9, 2021 •

edited

Loading

guigueb Mar 9, 2021 •

edited

Loading

dakahn Mar 9, 2021 •

edited

Loading

guigueb commented Mar 9, 2021 •

edited

Loading

guigueb commented Mar 10, 2021 •

edited

Loading

guigueb commented Mar 10, 2021 •

edited

Loading