feat(navigation-logo): add heading level #9054

josercarcamo · 2024-04-03T20:27:23Z

Related Issue: #7075

Summary

Added the "headingLevel" property to navigation-logo, similar to Panel.

driskull

driskull · 2024-04-03T20:48:55Z

packages/calcite-components/src/components/navigation-logo/navigation-logo.tsx

+    const headingNode = heading ? (
+      <Heading class={CSS.heading} level={headingLevel}>
+        <span
+          aria-label={this.heading}


I don't think we need an aria-label here. The heading element should already provide enough information.

Can we remove the span and add these attributes on the <heading />?

driskull · 2024-04-03T20:49:52Z

packages/calcite-components/src/components/navigation-logo/navigation-logo.tsx

+    ) : null;
+
+    const descriptionNode = description ? (
+      <span aria-label={this.description} class={CSS.description} key={CSS.description}>


Whats the need for the aria-label here as well?

I was just trying to keep the same sematics as what was in the original code, but I've removed it.

…cessary attributes

…:Esri/calcite-design-system into josercarcamo/7075-heading-level-nav-logo

geospatialem · 2024-04-03T22:39:48Z

packages/calcite-components/src/components/navigation-logo/navigation-logo.tsx

@@ -59,6 +60,11 @@ export class CalciteNavigationLogo implements LoadableComponent {
  /** Specifies the `src` to an image. */
  @Prop() thumbnail: string;

+  /**
+   * Specifies the number at which section headings should start.


@josercarcamo Very timely! @DitwanP made similar changes across the components earlier today. CC'ing him for editorial review on the headingLevel doc update.

@josercarcamo we are going with this description for headingLevel prop everywhere else, you can also add it here as well for consistency.

"Specifies the heading level of the component's heading for proper document structure, without affecting visual styling."

anveshmekala · 2024-04-03T22:59:00Z

Nitpick: PR title should be feat since this is an enhancement.

…tent with updated doc

driskull

Can you add an test within the e2e test that tests the headingLevel? This would be using the reflects common test helper. You can see an example in the block.e2e.ts.

 describe("reflects", () => {
    reflects("calcite-block", [
      {
        propertyName: "collapsible",
        value: true,
      },
      {
        propertyName: "headingLevel",
        value: 2,
      },

Also a "defaults" test would be good too.

describe("defaults", () => {
    defaults("calcite-block", [
      {
        propertyName: "collapsible",
        defaultValue: false,
      },
      {
        propertyName: "headingLevel",
        defaultValue: undefined,
      },

Can you also add a storybook story test in the navigation-logo.stories.ts that tests with a headingLevel applied? This way we can make sure different heading levels don't render differently.

Something like:

export const headingLevel_TestOnly = (): string => html`
 <calcite-navigation-logo
      heading="ArcGIS Online"
      heading-level="2"
      description="City of AcmeCo"
      thumbnail="${placeholderImage({ width: 50, height: 50 })}"
    />
`;

driskull · 2024-04-03T23:18:22Z

packages/calcite-components/src/components/navigation-logo/navigation-logo.tsx

+    ) : null;
+
+    return headingNode || descriptionNode ? (
+      <div class={CSS.container}>


nitpick: should we give this one a key for consistency? <div class={CSS.container} key={CSS.container}>. up to you :)

… review

driskull

Looks good to me! 👍

🤖 I have created a release *beep* *boop* --- <details><summary>@esri/calcite-design-tokens: 2.2.0</summary> ## [2.2.0](https://github.com/Esri/calcite-design-system/compare/@esri/calcite-design-tokens@2.1.2...@esri/calcite-design-tokens@2.2.0) (2024-04-30) ### Features * Ensure all components inherit font-family ([#8388](#8388)) ([90f8923](90f8923)) </details> <details><summary>@esri/eslint-plugin-calcite-components: 1.2.0</summary> ## [1.2.0](https://github.com/Esri/calcite-design-system/compare/@esri/eslint-plugin-calcite-components@1.1.0...@esri/eslint-plugin-calcite-components@1.2.0) (2024-04-30) ### Features * **enforce-ref-last-prop:** Add fixer ([#8671](#8671)) ([688bc51](688bc51)) ### Bug Fixes * **slider:** Improve snapping + step logic ([#8169](#8169)) ([8b83042](8b83042)) </details> <details><summary>@esri/calcite-components: 2.8.0</summary> ## [2.8.0](https://github.com/Esri/calcite-design-system/compare/@esri/calcite-components@2.7.1...@esri/calcite-components@2.8.0) (2024-04-30) ### Features * Add calciteInvalid event for form validation ([#9211](#9211)) ([a504508](a504508)) * Add validity property to form components ([#9210](#9210)) ([e49902b](e49902b)) * **carousel:** Add Carousel and Carousel Item components ([#8995](#8995)) ([b1ba428](b1ba428)) * **enforce-ref-last-prop:** Add fixer ([#8671](#8671)) ([688bc51](688bc51)) * Ensure all components inherit font-family ([#8388](#8388)) ([90f8923](90f8923)) * **input-time-zone:** Add readonly support ([#9111](#9111)) ([153a122](153a122)) * **input-time-zone:** Allow clearing value ([#9168](#9168)) ([193bb7d](193bb7d)) * **list-item-group:** Update list item group styles ([#9072](#9072)) ([c734849](c734849)) * **navigation-logo:** Add heading level ([#9054](#9054)) ([c2beba8](c2beba8)) * **pagination:** Add navigation methods ([#9154](#9154)) ([5ca6a5f](5ca6a5f)) * **slider:** Add support for custom label formatting ([#9179](#9179)) ([710d1ee](710d1ee)) * **slider:** Allow configuring fill behavior ([#9170](#9170)) ([1b2cdbf](1b2cdbf)) * **tile, tile-group:** Support single, multi, single-persist, none selection modes and icon and border selection appearances ([#9159](#9159)) ([2d77470](2d77470)) ### Bug Fixes * **action:** Maintain equal height when text is not enabled in a small scale ([#9051](#9051)) ([407824a](407824a)) * **chip-group:** Improve programmatic Chip selection behavior ([#9213](#9213)) ([b7a4c77](b7a4c77)) * **combobox:** Update the focus and hover chevron icon color ([#9187](#9187)) ([a1317da](a1317da)) * **combobox, input-time-zone:** Improve readOnly behavior and styling ([#9222](#9222)) ([606d80f](606d80f)) * **combobox:** Fix aria-role for proper voiceover support ([#9039](#9039)) ([eebe8ca](eebe8ca)) * **combobox:** Update the focused chevron icon color ([#9202](#9202)) ([27ac02d](27ac02d)) * **date-picker:** Ensure `proximitySelectionDisabled` resets range on post-range-commit selection ([#9171](#9171)) ([f466b14](f466b14)) * **date-picker:** Restore focus on date when navigating month with arrow/page keys ([#9063](#9063)) ([db109e0](db109e0)) * Fix memory leak affecting components using conditionally-rendered slots ([#9208](#9208)) ([28701b6](28701b6)) * **input-date-picker:** Update the focus and hover chevron icon color ([#9146](#9146)) ([ca895e9](ca895e9)) * **input-time-zone:** Ensure selected item is properly displayed when there are other items with the same offset ([#9134](#9134)) ([1f94903](1f94903)) * **input, input-number, input-text, input-date-picker, input-time-picker, filter, menu-item:** Ignore canceled events and enforce cancellations where needed ([#8952](#8952)) ([d0fa977](d0fa977)) * **list, list-item, list-item-group:** Improve drag experience by indenting items ([#9169](#9169)) ([88aab49](88aab49)) * **list, list-item:** Support keyboard sorting in screen readers ([#9038](#9038)) ([b2e1b9b](b2e1b9b)) * **list:** Closed list-items no longer render extra space ([#9031](#9031)) ([3985004](3985004)) * **panel:** Support cancelling the esc key event to prevent closing a panel ([#9032](#9032)) ([ecfa5f1](ecfa5f1)) * **preset:** Update the focus outline color ([#9098](#9098)) ([725f47c](725f47c)) * **radio-button:** Display validation message in parent group ([#9218](#9218)) ([b1e869f](b1e869f)) * **select:** Update the focus and hover chevron icon color and select focus and hover background color ([#9160](#9160)) ([b187340](b187340)) * **slider:** Fix fill placement and tick styling when ranged ([#9204](#9204)) ([a84d831](a84d831)) * **slider:** Improve snapping + step logic ([#8169](#8169)) ([8b83042](8b83042)) * **slider:** Rename `highlightPlacement` to `fillPlacement` ([#9195](#9195)) ([9e5a713](9e5a713)) * **slider:** Style ticks according to the fill placement ([#9196](#9196)) ([5eb4e10](5eb4e10)) * **stepper-item:** Remove delay in highlighting item ([#8996](#8996)) ([bcf23dd](bcf23dd)) * **tree-item:** Do not select when chevron clicked ([#9115](#9115)) ([99ad8f1](99ad8f1)) ### Dependencies * The following workspace dependencies were updated * devDependencies * @esri/calcite-design-tokens bumped from ^2.2.0-next.0 to ^2.2.0 * @esri/eslint-plugin-calcite-components bumped from ^1.2.0-next.1 to ^1.2.0 </details> <details><summary>@esri/calcite-components-angular: 2.8.0</summary> ## [2.8.0](https://github.com/Esri/calcite-design-system/compare/@esri/calcite-components-angular@2.7.1...@esri/calcite-components-angular@2.8.0) (2024-04-30) ### Miscellaneous Chores * **@esri/calcite-components-angular:** Synchronize components versions ### Dependencies * The following workspace dependencies were updated * dependencies * @esri/calcite-components bumped from ^2.8.0-next.20 to ^2.8.0 </details> <details><summary>@esri/calcite-components-react: 2.8.0</summary> ## [2.8.0](https://github.com/Esri/calcite-design-system/compare/@esri/calcite-components-react@2.7.1...@esri/calcite-components-react@2.8.0) (2024-04-30) ### Miscellaneous Chores * **@esri/calcite-components-react:** Synchronize components versions ### Dependencies * The following workspace dependencies were updated * dependencies * @esri/calcite-components bumped from ^2.8.0-next.20 to ^2.8.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>

fix(navigation-logo): adding heading level

4b63dce

josercarcamo requested a review from a team as a code owner April 3, 2024 20:27

github-actions bot assigned josercarcamo Apr 3, 2024

github-actions bot added the bug Bug reports for broken functionality. Issues should include a reproduction of the bug. label Apr 3, 2024

Merge branch 'main' into josercarcamo/7075-heading-level-nav-logo

929a2cf

driskull requested changes Apr 3, 2024

View reviewed changes

josercarcamo and others added 3 commits April 3, 2024 14:39

Merge branch 'main' into josercarcamo/7075-heading-level-nav-logo

41ae331

fix(navigation-logo): Refactors code to remove wrapping span and unne…

2f393e9

…cessary attributes

Merge branch 'josercarcamo/7075-heading-level-nav-logo' of github.com…

0bf0fad

…:Esri/calcite-design-system into josercarcamo/7075-heading-level-nav-logo

geospatialem reviewed Apr 3, 2024

View reviewed changes

josercarcamo added the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Apr 3, 2024

josercarcamo changed the title ~~fix(navigation-logo): adding heading level~~ feat(navigation-logo): adding heading level Apr 3, 2024

feat(navigation-logo): Changing comment for headingLevel to be consis…

36b2d47

…tent with updated doc

github-actions bot added the enhancement Issues tied to a new feature or request. label Apr 3, 2024

driskull requested changes Apr 3, 2024

View reviewed changes

feat(navigation-logo): adding test cases and 'key' attribute per code…

eebdd9b

… review

driskull approved these changes Apr 5, 2024

View reviewed changes

josercarcamo added 2 commits April 8, 2024 12:47

Merge branch 'main' into josercarcamo/7075-heading-level-nav-logo

ce4f385

Merge branch 'main' into josercarcamo/7075-heading-level-nav-logo

675f3fd

jcfranco changed the title ~~feat(navigation-logo): adding heading level~~ feat(navigation-logo): add heading level Apr 10, 2024

Merge branch 'main' into josercarcamo/7075-heading-level-nav-logo

9acb0e0

josercarcamo 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 Apr 11, 2024

josercarcamo merged commit c2beba8 into main Apr 11, 2024
16 checks passed

josercarcamo deleted the josercarcamo/7075-heading-level-nav-logo branch April 11, 2024 21:05

josercarcamo added the 3 - installed Issues that have been merged to master branch and are ready for final confirmation. label Apr 11, 2024

github-actions bot added this to the 2024-04-30 - Apr Release milestone Apr 11, 2024

josercarcamo removed the 3 - installed Issues that have been merged to master branch and are ready for final confirmation. label Apr 11, 2024

calcite-admin mentioned this pull request Apr 11, 2024

chore: release main #9050

Merged

calcite-admin mentioned this pull request May 14, 2024

chore: release main #9237

Merged

calcite-admin mentioned this pull request May 28, 2024

chore: release hotfix #9438

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(navigation-logo): add heading level #9054

feat(navigation-logo): add heading level #9054

josercarcamo commented Apr 3, 2024

driskull left a comment

driskull Apr 3, 2024

driskull Apr 3, 2024

driskull Apr 3, 2024

josercarcamo Apr 3, 2024

geospatialem Apr 3, 2024

DitwanP Apr 3, 2024

anveshmekala commented Apr 3, 2024

driskull left a comment •

edited

Loading

driskull Apr 3, 2024

driskull left a comment

feat(navigation-logo): add heading level #9054

feat(navigation-logo): add heading level #9054

Conversation

josercarcamo commented Apr 3, 2024

Summary

driskull left a comment

Choose a reason for hiding this comment

driskull Apr 3, 2024

Choose a reason for hiding this comment

driskull Apr 3, 2024

Choose a reason for hiding this comment

driskull Apr 3, 2024

Choose a reason for hiding this comment

josercarcamo Apr 3, 2024

Choose a reason for hiding this comment

geospatialem Apr 3, 2024

Choose a reason for hiding this comment

DitwanP Apr 3, 2024

Choose a reason for hiding this comment

anveshmekala commented Apr 3, 2024

driskull left a comment • edited Loading

Choose a reason for hiding this comment

driskull Apr 3, 2024

Choose a reason for hiding this comment

driskull left a comment

Choose a reason for hiding this comment

driskull left a comment •

edited

Loading