fix(block): display correct header spacing when heading or description are present by aPreciado88 · Pull Request #9924 · Esri/calcite-design-system

aPreciado88 · 2024-07-31T22:31:18Z

Related Issue: #9920

Summary

Update block component to display correct header spacing when heading or description properties are present.

driskull

Changes look good, just missing a few things 👍

driskull · 2024-07-31T23:13:49Z

 }

-:host([heading]),
 :host([description]) {


[description] would also be an issue here because it isn't reflected either. Should this css be removed?

driskull · 2024-07-31T23:15:37Z


    const headerContent = (
-      <header class={CSS.header} id={IDS.header}>
+      <header class={{ [CSS.header]: true, [CSS.headerPadding]: !!heading }} id={IDS.header}>


should this be !!(heading || description)?

Suggested change

<header class={{ [CSS.header]: true, [CSS.headerPadding]: !!heading }} id={IDS.header}>

<header class={{ [CSS.header]: true, [CSS.headerPadding]: !!(heading || description) }} id={IDS.header}>

driskull · 2024-07-31T23:17:05Z

  @apply justify-start;
 }

+.header-padding {


nitpick: maybe this should be named something like .header--present or .header--enabled?

aPreciado88 · 2024-07-31T23:27:45Z

@driskull I noticed that if I update the heading prop in block.tsx to @Prop({ reflect: true }) heading!: string; then the css will work, without any of the changes above. Is there a reason why it's declared as @Prop() heading!: string; ?
description is also declared as @Prop() description: string;. Should the fix be to add { reflect: true } to both props?

driskull · 2024-07-31T23:30:17Z

@aPreciado88 no, we don't want to reflect all properties. In cases where the property is just content , we don't want to reflect. https://github.com/Esri/calcite-design-system/tree/dev/packages/calcite-components/conventions#reflecting-to-attributes

…ding-is-present

…n are present

…esent' of github.com:Esri/calcite-design-system into aPreciado88/9920-spacing-not-honored-when-heading-is-present

aPreciado88 · 2024-07-31T23:52:49Z

@driskull I updated with the requested changes.

driskull

@aPreciado88 can we add a e2e test that checks for the css class when a heading or description is present via their properties?

Changes look good to me. Lets wait for @jcfranco to review before merging.

jcfranco

Once we have screenshot test coverage, this is good to merge! 🚀

jcfranco · 2024-08-01T16:58:10Z

  @apply justify-start;
 }

+.header--present {


Nitpick: I believe we use .<part>--has-text in other components for similar contexts.

jcfranco · 2024-08-01T17:50:06Z

      expect(actionAssignedSlot).toBe(SLOTS.headerMenuActions);
    });
+
+    it("applies correct header spacing when heading or description properties are present", async () => {


This is fine for now until we add extra checks to ensure styles use reflected props. After that, we should rely on screenshot tests. From the assertion perspective, it is not guaranteed that changes in the CSS class will be detected by this test, meaning its implementation could change without causing a test failure. cc @driskull

Agreed.

This is a sufficient test for now but ideally a screenshot test should be the real test.

@jcfranco Should I create a follow up issue to add a screenshot test?

@jcfranco I created this follow up issue to remove this E2E test once the reflected prop checks are in place.

…ding-is-present

…n are present (#9924) **Related Issue:** [#9920](#9920) ## Summary Update `block` component to display correct header spacing when `heading` or `description` properties are present. --------- Co-authored-by: Matt Driscoll <mdriscoll@esri.com>

🤖 I have created a release *beep* *boop* --- <details><summary>@esri/calcite-components: 2.11.1</summary> ## [2.11.1](https://github.com/Esri/calcite-design-system/compare/@esri/calcite-components@2.11.0...@esri/calcite-components@2.11.1) (2024-08-02) ### Bug Fixes * **block:** Display correct header spacing when heading or description are present ([#9924](#9924)) ([d8f1077](d8f1077)) * **panel, flow-item:** Fix header padding regression ([#9936](#9936)) ([90e9368](90e9368)) </details> <details><summary>@esri/calcite-components-angular: 2.11.1</summary> ## [2.11.1](https://github.com/Esri/calcite-design-system/compare/@esri/calcite-components-angular@2.11.0...@esri/calcite-components-angular@2.11.1) (2024-08-02) ### Miscellaneous Chores * **@esri/calcite-components-angular:** Synchronize components versions ### Dependencies * The following workspace dependencies were updated * dependencies * @esri/calcite-components bumped from ^2.11.0 to ^2.11.1 </details> <details><summary>@esri/calcite-components-react: 2.11.1</summary> ## [2.11.1](https://github.com/Esri/calcite-design-system/compare/@esri/calcite-components-react@2.11.0...@esri/calcite-components-react@2.11.1) (2024-08-02) ### Miscellaneous Chores * **@esri/calcite-components-react:** Synchronize components versions ### Dependencies * The following workspace dependencies were updated * dependencies * @esri/calcite-components bumped from ^2.11.0 to ^2.11.1 </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

🤖 I have created a release *beep* *boop* --- <details><summary>@esri/calcite-components: 2.11.1</summary> [2.11.1](https://github.com/Esri/calcite-design-system/compare/@esri/calcite-components@2.11.0...@esri/calcite-components@2.11.1) (2024-08-02) * **block:** Display correct header spacing when heading or description are present ([#9924](#9924)) ([d8f1077](d8f1077)) * **panel, flow-item:** Fix header padding regression ([#9936](#9936)) ([90e9368](90e9368)) </details> <details><summary>@esri/calcite-components-angular: 2.11.1</summary> [2.11.1](https://github.com/Esri/calcite-design-system/compare/@esri/calcite-components-angular@2.11.0...@esri/calcite-components-angular@2.11.1) (2024-08-02) * **@esri/calcite-components-angular:** Synchronize components versions * The following workspace dependencies were updated * dependencies * @esri/calcite-components bumped from ^2.11.0 to ^2.11.1 </details> <details><summary>@esri/calcite-components-react: 2.11.1</summary> [2.11.1](https://github.com/Esri/calcite-design-system/compare/@esri/calcite-components-react@2.11.0...@esri/calcite-components-react@2.11.1) (2024-08-02) * **@esri/calcite-components-react:** Synchronize components versions * The following workspace dependencies were updated * dependencies * @esri/calcite-components bumped from ^2.11.0 to ^2.11.1 </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

**Related Issue:** #9952 ## Summary Cherry-pick the release commit from `main`. --- <details><summary>@esri/calcite-components: 2.11.1</summary> [2.11.1](https://github.com/Esri/calcite-design-system/compare/@esri/calcite-components@2.11.0...@esri/calcite-components@2.11.1) (2024-08-02) * **block:** Display correct header spacing when heading or description are present ([#9924](#9924)) ([d8f1077](d8f1077)) * **panel, flow-item:** Fix header padding regression ([#9936](#9936)) ([90e9368](90e9368)) </details> <details><summary>@esri/calcite-components-angular: 2.11.1</summary> [2.11.1](https://github.com/Esri/calcite-design-system/compare/@esri/calcite-components-angular@2.11.0...@esri/calcite-components-angular@2.11.1) (2024-08-02) * **@esri/calcite-components-angular:** Synchronize components versions * The following workspace dependencies were updated * dependencies * @esri/calcite-components bumped from ^2.11.0 to ^2.11.1 </details> <details><summary>@esri/calcite-components-react: 2.11.1</summary> [2.11.1](https://github.com/Esri/calcite-design-system/compare/@esri/calcite-components-react@2.11.0...@esri/calcite-components-react@2.11.1) (2024-08-02) * **@esri/calcite-components-react:** Synchronize components versions * The following workspace dependencies were updated * dependencies * @esri/calcite-components bumped from ^2.11.0 to ^2.11.1 </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: Calcite Admin <calcite-admin@esri.com>

fix(block): display correct spacing when heading is present

2beee79

github-actions Bot assigned aPreciado88 Jul 31, 2024

github-actions Bot added the bug Bug reports for broken functionality. Issues should include a reproduction of the bug. label Jul 31, 2024

aPreciado88 added the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Jul 31, 2024

driskull requested changes Jul 31, 2024

View reviewed changes

driskull reviewed Jul 31, 2024

View reviewed changes

driskull and others added 3 commits July 31, 2024 16:36

Merge branch 'dev' into aPreciado88/9920-spacing-not-honored-when-hea…

1abfb72

…ding-is-present

fix(block): display correct header spacing when heading or descriptio…

f311b11

…n are present

Merge branch 'aPreciado88/9920-spacing-not-honored-when-heading-is-pr…

8150fea

…esent' of github.com:Esri/calcite-design-system into aPreciado88/9920-spacing-not-honored-when-heading-is-present

aPreciado88 requested a review from driskull July 31, 2024 23:52

aPreciado88 changed the title ~~fix(block): display correct header spacing when heading is present~~ fix(block): display correct header spacing when heading or description are present Jul 31, 2024

aPreciado88 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 Jul 31, 2024

driskull approved these changes Jul 31, 2024

View reviewed changes

jcfranco approved these changes Aug 1, 2024

View reviewed changes

fix(block): add end to end test

550e056

jcfranco reviewed Aug 1, 2024

View reviewed changes

aPreciado88 added 2 commits August 1, 2024 10:50

fix(block): update header class name

379dee9

Merge branch 'dev' into aPreciado88/9920-spacing-not-honored-when-hea…

f366f7d

…ding-is-present

aPreciado88 added pr ready for visual snapshots Adding this label will run visual snapshot testing. skip visual snapshots Pull requests that do not need visual regression testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Aug 1, 2024

aPreciado88 merged commit 23d67b4 into dev Aug 1, 2024

aPreciado88 deleted the aPreciado88/9920-spacing-not-honored-when-heading-is-present branch August 1, 2024 19:40

github-actions Bot added this to the 2.11.1 patch milestone Aug 1, 2024

benelan mentioned this pull request Aug 2, 2024

chore: cherry-pick patch commits #9950

Merged

calcite-admin mentioned this pull request Aug 2, 2024

chore: release main #9952

Merged

benelan mentioned this pull request Aug 2, 2024

chore: release main (#9952) #9954

Merged

benelan mentioned this pull request Aug 28, 2024

chore: release main #10171

Merged

calcite-admin mentioned this pull request Sep 30, 2024

chore: release main #10457

Merged

	<header class={{ [CSS.header]: true, [CSS.headerPadding]: !!heading }} id={IDS.header}>
	<header class={{ [CSS.header]: true, [CSS.headerPadding]: !!(heading \|\| description) }} id={IDS.header}>

Conversation

aPreciado88 commented Jul 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

driskull left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aPreciado88 commented Jul 31, 2024

Uh oh!

driskull commented Jul 31, 2024

Uh oh!

aPreciado88 commented Jul 31, 2024

Uh oh!

driskull left a comment

Choose a reason for hiding this comment

Uh oh!

jcfranco left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

aPreciado88 commented Jul 31, 2024 •

edited

Loading