fix(panel): Fix heading border when only text content is slotted #7491

driskull · 2023-08-08T22:34:58Z

Related Issue: #7489

Summary

Restores border-block-end by default.
Adds internal CSS variable for temporary usage
Issue to add panel collapsing support: Support panel collapsing #7498
Adds story tests

driskull · 2023-08-08T22:40:42Z

packages/calcite-components/src/utils/dom.ts

+    .map((node) => node.textContent)
+    .join("")
+    .trim();
+}


@anveshmekala I think this utility could be used for...

calcite-design-system/packages/calcite-components/src/components/text-area/text-area.tsx

Lines 406 to 415 in bb07831

contentSlotChangeHandler = (): void => {

if (!this.value) {

const nodes = this.el.childNodes;

nodes.forEach((el) => {

if (el.nodeName === "#text") {

this.value = el.nodeValue.trim();

}

});

}

};

What do you think?

I'm a little confused about the logic above though, it seems like the last textNode will be set as this.value and any previous nodes would be overwritten.

Good observation. I agree that it does seem suspect.

@anveshmekala I think this utility could be used for...

calcite-design-system/packages/calcite-components/src/components/text-area/text-area.tsx

Lines 406 to 415 in bb07831

contentSlotChangeHandler = (): void => {

if (!this.value) {

const nodes = this.el.childNodes;

nodes.forEach((el) => {

if (el.nodeName === "#text") {

this.value = el.nodeValue.trim();

}

});

}

};

What do you think?

This works well for text-area use-case of extracting text. Regarding the logic , if we have multiple textNodes then yes the last node will be set as value. I wonder if there could be a case where one can have multiple textNodes . Multi-line text will still be considered as one textNode i believe.

@anveshmekala I think this breaks the logic:

<calcite-text-area> Hello world hi</calcite-text-area>

https://codepen.io/driskull/pen/oNQREpy?editors=1111

It seems to work but could you double check?

it works with multi line text including special characters.

jcfranco

Looking good, but can we separate the utils that aren't related to the fix for now?

packages/calcite-components/src/utils/dom.ts

packages/calcite-components/src/components/panel/panel.tsx

jcfranco · 2023-08-08T22:54:55Z

packages/calcite-components/src/utils/dom.ts

+    .map((node) => node.textContent)
+    .join("")
+    .trim();
+}


Good observation. I agree that it does seem suspect.

macandcheese · 2023-08-08T23:34:35Z

This is a great update but I think a lot of the doc site use cases would still have the missing border - can we only hide one of the duplicate borders when footer content is populated or footer / header containers are intersecting?

driskull · 2023-08-08T23:35:59Z

I think a lot of the doc site use cases would still have the missing border

Can you explain why?

macandcheese · 2023-08-08T23:46:14Z

I think a lot of the doc site use cases would still have the missing border

Can you explain why?

Because they don't have text or any content in them. In an app use case, maybe a Panel is unpopulated before an element is added:

Previous behavior with totally empty Panel:

Current:

I think the fix here is still valid but maybe there is another way to determine when to remove the border, if footer / header overlap remove it?

driskull · 2023-08-08T23:51:04Z

I think the fix here is still valid but maybe there is another way to determine when to remove the border, if footer / header overlap remove it?

We'll have to brainstorm because it's not just the footer that is the problem.

When a popup is collapsed, we don't want the border line showing here as it would divide nothing and put a line through the pointer tail.

driskull · 2023-08-08T23:59:58Z

Maybe we need to build collapsing into the panel/flow-item to handle this correctly? Then it would only be removed when collapsed?

driskull · 2023-08-09T00:02:19Z

Another option would be the ability to turn off the border when necessary through a CSS variable.

jcfranco

Awesome! 🚀

packages/calcite-components/src/components/panel/panel.stories.ts

macandcheese · 2023-08-09T18:54:44Z

packages/calcite-components/src/components/panel/panel.scss

@@ -12,6 +12,7 @@

  --calcite-min-header-height: calc(var(--calcite-icon-size) * 3);
  --calcite-panel-footer-padding: theme("spacing.2");
+  --calcite-internal-panel-header-border-block-end: 1px solid var(--calcite-ui-border-3);


Can the -internal be at the end? I think that's the pattern elsewhere - maybe we can add that to conventions?

calcite-design-system/packages/calcite-components/src/components/chip/chip.scss

Line 3 in bb07831

--calcite-chip-spacing-s-internal: theme("spacing[1]");

☝️ this is actually inconsistent with some other internal CSS props. 😅

I think internal should be more prominent in the name. This might be easier to spot if it's part of the prefix.

maybe we can add that to conventions?

Excellent idea. I don't think we have this documented at the moment.

Yeah I looked at other internal css props and some start with internal and some end with internal. Can we open an issue for a convention and refactoring?

LOL - yeah it's about 50/50. I feel like it's easier to spot at the end with nothing following it but... as long as its consistent either way.

I feel like we should follow events as they are prefixed with calciteInternal.

driskull · 2023-08-09T20:01:45Z

@macandcheese @jcfranco good to merge?

jcfranco · 2023-08-09T20:50:42Z

Good to murge! 🚀

🤖 I have created a release *beep* *boop* --- <details><summary>@esri/calcite-components: 1.6.0</summary> ## [1.6.0](https://github.com/Esri/calcite-design-system/compare/@esri/[email protected]...@esri/[email protected]) (2023-08-15) ### Features * **action-bar:** Add "actions-end" slot (deprecates "bottom-actions") ([#7435](#7435)) ([1bf14ff](1bf14ff)) ### Bug Fixes * **block:** Defaults the status icon to `scale=s` ([#7503](#7503)) ([e1aee99](e1aee99)) * **button,card,fab,inline-editable:** Provides context to AT users when loading ([#7257](#7257)) ([df33eda](df33eda)) * **chip-group:** Add existence checks ([#7487](#7487)) ([33225a7](33225a7)) * **combobox:** Prevents navigation list with Space key ([#7505](#7505)) ([58e2ff2](58e2ff2)) * **panel:** Fix heading border when only text content is slotted ([#7491](#7491)) ([7704400](7704400)) * **progress:** Completes animation for `dir='rtl'` ([#7511](#7511)) ([c5d6ada](c5d6ada)) * **scrim:** Handle slotted children correctly ([#7477](#7477)) ([c5ce008](c5ce008)) * **scrim:** Render text content inside scrim ([#7509](#7509)) ([643ce5d](643ce5d)) * **slider:** Rerender ticks when prop is modified ([#7439](#7439)) ([20058a9](20058a9)) * **tree:** Selects all child items when selection-mode is set to ancestors ([#7518](#7518)) ([f1eef84](f1eef84)) </details> <details><summary>@esri/calcite-components-react: 1.6.0</summary> ## [1.6.0](https://github.com/Esri/calcite-design-system/compare/@esri/[email protected]...@esri/[email protected]) (2023-08-15) ### Bug Fixes * Prevent vitetest/ssr errors due to defining components on the server ([#7521](#7521)) ([046672e](046672e)) ### Dependencies * The following workspace dependencies were updated * dependencies * @esri/calcite-components bumped from ^1.6.0-next.7 to ^1.6.0 </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

driskull added 2 commits August 8, 2023 15:21

fix(panel): Fix heading border when only text content is slotted. #7489

62c2776

tests

bae418b

github-actions bot assigned driskull Aug 8, 2023

github-actions bot added the bug Bug reports for broken functionality. Issues should include a reproduction of the bug. label Aug 8, 2023

fix story

ef54098

driskull commented Aug 8, 2023

View reviewed changes

driskull marked this pull request as ready for review August 8, 2023 22:42

driskull requested a review from a team as a code owner August 8, 2023 22:42

driskull added the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Aug 8, 2023

jcfranco reviewed Aug 8, 2023

View reviewed changes

review fixes

46df36d

driskull added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Aug 9, 2023

driskull mentioned this pull request Aug 9, 2023

Support panel collapsing #7498

Closed

3 tasks

cleanup stories

dbc6c3d

driskull added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Aug 9, 2023

driskull requested review from jcfranco and macandcheese August 9, 2023 18:50

jcfranco approved these changes Aug 9, 2023

View reviewed changes

packages/calcite-components/src/components/panel/panel.stories.ts Outdated Show resolved Hide resolved

macandcheese reviewed Aug 9, 2023

View reviewed changes

review fixes

1936359

driskull added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Aug 9, 2023

driskull merged commit 7704400 into main Aug 9, 2023

driskull deleted the dris0000/panel-text-content-fix branch August 9, 2023 20:51

github-actions bot added this to the 1.6.0 - Low risk issues only milestone Aug 9, 2023

calcite-admin mentioned this pull request Aug 9, 2023

chore: release main #7485

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(panel): Fix heading border when only text content is slotted #7491

fix(panel): Fix heading border when only text content is slotted #7491

driskull commented Aug 8, 2023 •

edited

Loading

driskull Aug 8, 2023

driskull Aug 8, 2023

jcfranco Aug 8, 2023

anveshmekala Aug 9, 2023

driskull Aug 9, 2023

driskull Aug 9, 2023

anveshmekala Aug 9, 2023

jcfranco left a comment

jcfranco Aug 8, 2023

macandcheese commented Aug 8, 2023 •

edited

Loading

driskull commented Aug 8, 2023

macandcheese commented Aug 8, 2023 •

edited

Loading

driskull commented Aug 8, 2023

driskull commented Aug 8, 2023

driskull commented Aug 9, 2023

jcfranco left a comment

macandcheese Aug 9, 2023

jcfranco Aug 9, 2023

jcfranco Aug 9, 2023

driskull Aug 9, 2023

macandcheese Aug 9, 2023

driskull Aug 9, 2023

macandcheese Aug 9, 2023

driskull commented Aug 9, 2023

jcfranco commented Aug 9, 2023

	contentSlotChangeHandler = (): void => {
	if (!this.value) {
	const nodes = this.el.childNodes;
	nodes.forEach((el) => {
	if (el.nodeName === "#text") {
	this.value = el.nodeValue.trim();
	}
	});
	}
	};

fix(panel): Fix heading border when only text content is slotted #7491

fix(panel): Fix heading border when only text content is slotted #7491

Conversation

driskull commented Aug 8, 2023 • edited Loading

Summary

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

Choose a reason for hiding this comment

jcfranco left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

macandcheese commented Aug 8, 2023 • edited Loading

driskull commented Aug 8, 2023

macandcheese commented Aug 8, 2023 • edited Loading

driskull commented Aug 8, 2023

driskull commented Aug 8, 2023

driskull commented Aug 9, 2023

jcfranco left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

driskull commented Aug 9, 2023

jcfranco commented Aug 9, 2023

driskull commented Aug 8, 2023 •

edited

Loading

macandcheese commented Aug 8, 2023 •

edited

Loading

macandcheese commented Aug 8, 2023 •

edited

Loading