#1646 Panel accordions are not using the carbon standards #1648

srikant-ch5 · 2023-12-14T14:30:50Z

Developer's Certificate of Origin 1.1

   By making a contribution to this project, I certify that:

   (a) The contribution was created in whole or in part by me and I
       have the right to submit it under the Apache License 2.0; or

   (b) The contribution is based upon previous work that, to the best
       of my knowledge, is covered under an appropriate open source
       license and I have the right under that license to submit that
       work with modifications, whether created in whole or in part
       by me, under the same open source license (unless I am
       permitted to submit under a different license), as indicated
       in the file; or

   (c) The contribution was provided directly to me by some other
       person who certified (a), (b) or (c) and I have not modified
       it.

   (d) I understand and agree that this project and the contribution
       are public and that a record of the contribution (including all
       personal information I submit with it, including my sign-off) is
       maintained indefinitely and may be redistributed consistent with
       this project or the open source license(s) involved.

Signed-off-by: srikant <[email protected]>

…cases by including tabContent Signed-off-by: srikant <[email protected]>

Signed-off-by: srikant <[email protected]>

matthoward366 · 2023-12-19T17:15:47Z

A few things

Nested Twisty panels don't seem to be working.
Is there a reason we're closing open panels when a new panel opens? I think we can leave panels open now?
When opening a panel it's kind of jerky and flickers. Do you also see this?
When using selectors in tests like button.bx--accordion__heading are we able to define our own class instead? Ideally we'd keep most of the same classNames we had before. For example is it possible to use properties-category-title here?

srikant-ch5 · 2023-12-20T07:00:00Z

A few things

Nested Twisty panels don't seem to be working.

Yes, I have mentioned this in the issue Panel accordions are not using the carbon standards #1646, this is because of accrodion architecture and is tracked under issue https://github.ibm.com/ngp-twc/wdp-abstract-canvas/issues/2909

Is there a reason we're closing open panels when a new panel opens? I think we can leave panels open now?

I have included this to replicate previous panel behaviour I will update PR to leave panels open as default accordion behaviour.

When opening a panel it's kind of jerky and flickers. Do you also see this?

This issue can be resolved once we allow multiple panels to be opened.

When using selectors in tests like button.bx--accordion__heading are we able to define our own class instead? Ideally we'd keep most of the same classNames we had before. For example is it possible to use properties-category-title here?

I will update this PR taking this into consideration.

matthoward366 · 2023-12-20T12:27:55Z

We need to have 1 fixed before we can merge this PR. This would be a regression of the current behavior.

Signed-off-by: srikant <[email protected]>

…htFlyoutN

Signed-off-by: srikant <[email protected]>

srikant-ch5 · 2023-12-21T12:22:58Z

We need to have 1 fixed before we can merge this PR. This would be a regression of the current behavior.

Hi @matthoward366 Accordion Inside Accordion Issue is resolved.
I have replaced few classnames as they were before but for accordion button its same since its present at child level inside AccordionItem.

matthoward366 · 2024-01-03T22:04:48Z

Did you make the change for This issue can be resolved once we allow multiple panels to be opened.? I'm still only seeing 1 panel open at a time.

matthoward366 · 2024-01-03T22:08:58Z

canvas_modules/common-canvas/src/common-properties/components/editor-form/editor-form.scss

+		padding: $spacing-06 $spacing-05 $spacing-05;
+
+
+		.bx--accordion__item {


Can you give some details as to why we need to set each of these? I took out most of these a didn't notice an issue.

This spacing is to keep it consistent with Accordions included in twisty panel and also with previous layout

canvas/canvas_modules/common-canvas/src/common-properties/panels/twisty/twisty.scss

Line 27 in 59dc2d5

padding: $spacing-06 $spacing-05 $spacing-05;

This is fine but does this need to use the carbon class? Also, can you add a comment to why each of these carbon overrides are needed? For example why do we need to set transform: rotate(90deg);?

Since Accordion inside Accordions are not supported in Carbon Components carbon-design-system/carbon#4768 (comment) I am handling collapsed and expanded state to hide/unhide the inner accordion content and rotate accordion arrow present beside the header using these carbon classes.

matthoward366 · 2024-01-03T22:11:44Z

canvas_modules/common-canvas/src/common-properties/components/editor-form/editor-form.scss

-			border-bottom: 0;
+.properties-categories {  
+	.bx--accordion__item {
+		margin: 3 -1rem; // Remove 16px outer padding for accordion title and content


I'm seeing this in the browser.

Thanks for the feedback, I have removed this in my latest commit.

…lerts when clicked Signed-off-by: srikant <[email protected]>

srikant-ch5 · 2024-01-05T15:29:06Z

Did you make the change for This issue can be resolved once we allow multiple panels to be opened.? I'm still only seeing 1 panel open at a time.

HI @matthoward366 Now multiple panels can be opened at the same time and also can navigate to panels with alerts. Please review.

matthoward366 · 2024-01-05T22:55:33Z

canvas_modules/common-canvas/src/common-properties/components/editor-form/editor-form.jsx

+								// Open Tab with Alert Message when from Alerts Tab or a open Default Tab
+								open={ this.defaultOpenTab === tab.group || this.alertOpenTab === tab.group }
+								onHeadingClick={this._showCategoryPanel.bind(this, tab.group)}
+								className={`bx--accordion__item-${i} ${classNames("properties-category-content",


Why are we using bx--accordion__item-${i} for a classname? This seems odd.

I have removed this classname now for the content.

matthoward366 · 2024-01-05T22:57:53Z

canvas_modules/common-canvas/src/common-properties/components/editor-form/editor-form.scss

+		padding: $spacing-06 $spacing-05 $spacing-05;
+
+
+		.bx--accordion__item {


This is fine but does this need to use the carbon class? Also, can you add a comment to why each of these carbon overrides are needed? For example why do we need to set transform: rotate(90deg);?

Signed-off-by: srikant <[email protected]>

srikant-ch5 · 2024-04-16T14:40:45Z

Hi @nmgokhale @matthoward366 I have included Carbon 11 Accordions in this PR, please review and let me know in case of any changes. Thanks.

Signed-off-by: srikant <[email protected]>

nmgokhale · 2024-04-16T18:51:23Z

package-lock.json

@@ -0,0 +1,1199 @@
+{
+  "name": "canvas",


Delete package-lock.json file

Hi @nmgokhale I have deleted package-lock.json file and updated the PR.

Signed-off-by: srikant <[email protected]>

nmgokhale · 2024-04-17T23:00:33Z

@srikant-ch5

Comparing your PR against https://elyra-canvas-test-harness.u20youmx4sm.us-south.codeengine.appdomain.cloud/#/, there's a lot of spacing below category content. Compare the padding for properties-category-content class in your PR and test harness.

Comparing properties accordion with panel accordion, there are some differences in CSS -

In https://elyra-canvas-test-harness.u20youmx4sm.us-south.codeengine.appdomain.cloud/#/, only 1 category is open at a time whereas in your PR, I can open several categories. @matthoward366 Is this okay?

Signed-off-by: srikant <[email protected]>

srikant-ch5 · 2024-04-18T09:54:04Z

Hi @nmgokhale

I have adjusted spacing and padding to match with existing panels.
I have adjusted height and border colour of accordionitems.
As per our above conversation we are allowing multiple accordions to be open at once.
Please review above changes.

canvas_modules/common-canvas/src/common-properties/components/editor-form/editor-form.scss

nmgokhale · 2024-04-18T19:15:43Z

canvas_modules/common-canvas/src/common-properties/components/editor-form/editor-form.scss

+	}
+
+	// Remove Default borders for Accordions
+	.cds--accordion__item {


Replace cds--accordion__item with properties-category-content.

nmgokhale · 2024-04-18T19:19:05Z

canvas_modules/common-canvas/src/common-properties/components/editor-form/editor-form.scss

-	.cds--tabs__nav {
-		overflow-x: auto; // override carbon so scrollbar doesn't always show
+.properties-categories {
+	.cds--accordion__item--active .cds--accordion__content {


This should be .properties-category-content .cds--accordion__content

Adding .cds--accordion__item--active removes the CSS when multiple accordions are open.

nmgokhale · 2024-04-18T19:28:06Z

canvas_modules/common-canvas/src/common-properties/components/editor-form/editor-form.scss

-		}
-	}
-}
-
 .properties-category-title {


This class no longer exists in the code.

nmgokhale · 2024-04-18T19:31:04Z

canvas_modules/common-canvas/src/common-properties/components/editor-form/editor-form.scss

+	}
+
+	//Override default accordionitem height to match with previous panels
+	.cds--accordion__heading {


You don't need this css. You can specify <Accordion size="lg"> in editor-form.jsx file and it will add height 48px.

nmgokhale · 2024-04-18T19:46:29Z

canvas_modules/common-canvas/src/common-properties/components/editor-form/editor-form.scss

+
+	// Remove default accordion padding
+	.cds--accordion__item--active .cds--accordion__wrapper {
+		padding-block-end: 0px;


This is still adding some top padding. Change this to -

// Remove default accordion padding .properties-category-content .cds--accordion__wrapper { padding: 0; }

nmgokhale · 2024-04-18T19:55:06Z

canvas_modules/common-canvas/src/common-properties/components/editor-form/editor-form.scss

+.properties-categories {
+	.cds--accordion__item--active .cds--accordion__content {
+		// 24px padding between accordion title and content (issue #3106)
+		padding: $spacing-02 $spacing-05 $spacing-05;


Remove the comment on line 85.
This padding doesn't look correct. In test harness, there's padding: $spacing-05 added -

Signed-off-by: srikant <[email protected]>

…l work Signed-off-by: srikant <[email protected]>

srikant-ch5 · 2024-04-21T09:30:44Z

Hi @matthoward366 Could you please review Carbon 11 Accordion changes in this PR ?

matthoward366

Looks good

…yra-ai#1648) Signed-off-by: Matt Howard <[email protected]>

srikant-ch5 added 2 commits December 14, 2023 19:58

elyra-ai#1646 Panel accordions are not using the carbon standards

ef939bb

Signed-off-by: srikant <[email protected]>

elyra-ai#1646 Panel accordions - Resolve code-test and tearsheet test…

5a19d94

…cases by including tabContent Signed-off-by: srikant <[email protected]>

srikant-ch5 requested a review from JesusGuerrero December 15, 2023 07:36

Conditionally render panel accordions

8ef25bc

Signed-off-by: srikant <[email protected]>

JesusGuerrero requested review from matthoward366 and removed request for JesusGuerrero December 18, 2023 19:41

srikant-ch5 added 3 commits December 21, 2023 17:33

Fix Nested Accordion Issue and classnames

f8467ee

Signed-off-by: srikant <[email protected]>

Merge remote-tracking branch 'upstream/main' into 14295_Accordion_Rig…

89eb79c

…htFlyoutN

#elyra-ai#1646 Use default classnames

f3e7f9c

Signed-off-by: srikant <[email protected]>

srikant-ch5 force-pushed the 14295_Accordion_RightFlyoutN branch from 4a99a2b to f3e7f9c Compare December 21, 2023 12:18

Merge branch 'main' into 14295_Accordion_RightFlyoutN

57ae673

matthoward366 reviewed Jan 3, 2024

View reviewed changes

Enable Opening of Multiple Tabs, Open Default Tab and Open Tab with A…

ce5b0a0

…lerts when clicked Signed-off-by: srikant <[email protected]>

matthoward366 reviewed Jan 5, 2024

View reviewed changes

srikant-ch5 and others added 4 commits January 8, 2024 11:50

Remove bx classname from content and describe accordion inside accordion

cf70ce6

Signed-off-by: srikant <[email protected]>

Merge branch 'main' into 14295_Accordion_RightFlyoutN

1846351

Merge branch 'main' into 14295_Accordion_RightFlyoutN

0533b69

Merge branch 'main' into 14295_Accordion_RightFlyoutN

7e01dc8

srikant-ch5 mentioned this pull request Feb 28, 2024

Display Palette category content using keyboard #1714

Merged

srikant-ch5 added 3 commits February 28, 2024 11:55

Fix setTopLevelActiveGroup test case failure due to accordion

73119e9

Signed-off-by: srikant <[email protected]>

Include Carbon 11 Changes in Panel Accordions

3237bad

Signed-off-by: srikant <[email protected]>

Merge branch 'main' into 14295_Accordion_RightFlyoutN

b0f62e6

srikant-ch5 requested a review from nmgokhale April 16, 2024 14:41

srikant-ch5 added 2 commits April 16, 2024 20:27

Fix Build issues in editor-form files

c0fb47f

Signed-off-by: srikant <[email protected]>

Revert Copyright changes in editor-form.scss

f72d07f

Signed-off-by: srikant <[email protected]>

nmgokhale reviewed Apr 16, 2024

View reviewed changes

srikant-ch5 added 2 commits April 17, 2024 11:40

Merge branch 'main' into 14295_Accordion_RightFlyoutN

93e1b6c

Delete package-lock.json

4394438

Signed-off-by: srikant <[email protected]>

srikant-ch5 added 3 commits April 18, 2024 09:33

Merge branch 'main' into 14295_Accordion_RightFlyoutN

5a31522

Adjust Accordion height and border as per previous setup

9455f20

Signed-off-by: srikant <[email protected]>

Update Accordion border

789f4bb

Signed-off-by: srikant <[email protected]>

nmgokhale reviewed Apr 18, 2024

View reviewed changes

canvas_modules/common-canvas/src/common-properties/components/editor-form/editor-form.scss Show resolved Hide resolved

nmgokhale reviewed Apr 18, 2024

View reviewed changes

srikant-ch5 added 3 commits April 19, 2024 17:05

Fix css issues by replacing classnames

1b7280b

Signed-off-by: srikant <[email protected]>

Merge branch 'main' into 14295_Accordion_RightFlyoutN

283ad0e

Add cds--accordion__item class in editor-form css to make twisty pane…

2930b5b

…l work Signed-off-by: srikant <[email protected]>

nmgokhale approved these changes Apr 19, 2024

View reviewed changes

srikant-ch5 requested a review from matthoward366 April 21, 2024 09:24

Merge branch 'main' into 14295_Accordion_RightFlyoutN

a5131a8

matthoward366 approved these changes Apr 22, 2024

View reviewed changes

matthoward366 merged commit e45ca41 into elyra-ai:main Apr 22, 2024
3 checks passed

srikant-ch5 deleted the 14295_Accordion_RightFlyoutN branch June 7, 2024 05:36

matthoward366 pushed a commit to matthoward366/canvas that referenced this pull request Aug 5, 2024

elyra-ai#1646 Panel accordions are not using the carbon standards (el…

4110337

…yra-ai#1648) Signed-off-by: Matt Howard <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#1646 Panel accordions are not using the carbon standards #1648

#1646 Panel accordions are not using the carbon standards #1648

srikant-ch5 commented Dec 14, 2023

matthoward366 commented Dec 19, 2023 •

edited

Loading

srikant-ch5 commented Dec 20, 2023 •

edited

Loading

matthoward366 commented Dec 20, 2023

srikant-ch5 commented Dec 21, 2023 •

edited

Loading

matthoward366 commented Jan 3, 2024

matthoward366 Jan 3, 2024 •

edited

Loading

srikant-ch5 Jan 5, 2024

matthoward366 Jan 5, 2024

srikant-ch5 Jan 8, 2024

matthoward366 Jan 3, 2024

srikant-ch5 Jan 5, 2024

srikant-ch5 commented Jan 5, 2024 •

edited

Loading

matthoward366 Jan 5, 2024

srikant-ch5 Jan 8, 2024

matthoward366 Jan 5, 2024

srikant-ch5 commented Apr 16, 2024

nmgokhale Apr 16, 2024

srikant-ch5 Apr 17, 2024

nmgokhale commented Apr 17, 2024

srikant-ch5 commented Apr 18, 2024 •

edited

Loading

nmgokhale Apr 18, 2024

nmgokhale Apr 18, 2024 •

edited

Loading

nmgokhale Apr 18, 2024

nmgokhale Apr 18, 2024 •

edited

Loading

nmgokhale Apr 18, 2024 •

edited

Loading

nmgokhale Apr 18, 2024

srikant-ch5 commented Apr 21, 2024

matthoward366 left a comment

		padding: $spacing-06 $spacing-05 $spacing-05;


		.bx--accordion__item {

#1646 Panel accordions are not using the carbon standards #1648

#1646 Panel accordions are not using the carbon standards #1648

Conversation

srikant-ch5 commented Dec 14, 2023

matthoward366 commented Dec 19, 2023 • edited Loading

srikant-ch5 commented Dec 20, 2023 • edited Loading

matthoward366 commented Dec 20, 2023

srikant-ch5 commented Dec 21, 2023 • edited Loading

matthoward366 commented Jan 3, 2024

matthoward366 Jan 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srikant-ch5 commented Jan 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srikant-ch5 commented Apr 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nmgokhale commented Apr 17, 2024

srikant-ch5 commented Apr 18, 2024 • edited Loading

Choose a reason for hiding this comment

nmgokhale Apr 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nmgokhale Apr 18, 2024 • edited Loading

Choose a reason for hiding this comment

nmgokhale Apr 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srikant-ch5 commented Apr 21, 2024

matthoward366 left a comment

Choose a reason for hiding this comment

matthoward366 commented Dec 19, 2023 •

edited

Loading

srikant-ch5 commented Dec 20, 2023 •

edited

Loading

srikant-ch5 commented Dec 21, 2023 •

edited

Loading

matthoward366 Jan 3, 2024 •

edited

Loading

srikant-ch5 commented Jan 5, 2024 •

edited

Loading

srikant-ch5 commented Apr 18, 2024 •

edited

Loading

nmgokhale Apr 18, 2024 •

edited

Loading

nmgokhale Apr 18, 2024 •

edited

Loading

nmgokhale Apr 18, 2024 •

edited

Loading