Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 10 commits into from
Apr 11, 2024
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
4b63dce
fix(navigation-logo): adding heading level
josercarcamo Apr 3, 2024
929a2cf
Merge branch 'main' into josercarcamo/7075-heading-level-nav-logo
josercarcamo Apr 3, 2024
41ae331
Merge branch 'main' into josercarcamo/7075-heading-level-nav-logo
josercarcamo Apr 3, 2024
2f393e9
fix(navigation-logo): Refactors code to remove wrapping span and unne…
josercarcamo Apr 3, 2024
0bf0fad
Merge branch 'josercarcamo/7075-heading-level-nav-logo' of github.com…
josercarcamo Apr 3, 2024
36b2d47
feat(navigation-logo): Changing comment for headingLevel to be consis…
josercarcamo Apr 3, 2024
eebdd9b
feat(navigation-logo): adding test cases and 'key' attribute per code…
josercarcamo Apr 4, 2024
ce4f385
Merge branch 'main' into josercarcamo/7075-heading-level-nav-logo
josercarcamo Apr 8, 2024
675f3fd
Merge branch 'main' into josercarcamo/7075-heading-level-nav-logo
josercarcamo Apr 10, 2024
9acb0e0
Merge branch 'main' into josercarcamo/7075-heading-level-nav-logo
josercarcamo Apr 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions packages/calcite-components/src/components.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3264,6 +3264,10 @@ export namespace Components {
* Specifies heading text for the component, such as a product or organization name.
*/
"heading": string;
/**
* Specifies the number at which section headings should start.
*/
"headingLevel": HeadingLevel;
/**
* Specifies the URL destination of the component, which can be set as an absolute or relative path.
*/
Expand Down Expand Up @@ -10825,6 +10829,10 @@ declare namespace LocalJSX {
* Specifies heading text for the component, such as a product or organization name.
*/
"heading"?: string;
/**
* Specifies the number at which section headings should start.
*/
"headingLevel"?: HeadingLevel;
/**
* Specifies the URL destination of the component, which can be set as an absolute or relative path.
*/
Expand Down
61 changes: 39 additions & 22 deletions packages/calcite-components/src/components/navigation-logo/navigation-logo.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {
setComponentLoaded,
setUpLoadableComponent,
} from "../../utils/loadable";
import { Heading, HeadingLevel } from "../functional/Heading";

@Component({
tag: "calcite-navigation-logo",
Expand Down Expand Up @@ -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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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."

*/
@Prop({ reflect: true }) headingLevel: HeadingLevel;

//--------------------------------------------------------------------------
//
// Public Methods
Expand Down Expand Up @@ -107,34 +113,45 @@ export class CalciteNavigationLogo implements LoadableComponent {
return <calcite-icon class={CSS.icon} flipRtl={this.iconFlipRtl} icon={this.icon} scale="l" />;
}

renderHeaderContent(): VNode {
const { heading, headingLevel, description } = this;
const headingNode = heading ? (
<Heading class={CSS.heading} level={headingLevel}>
<span
aria-label={this.heading}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

class={{
[CSS.heading]: true,
[CSS.standalone]: !this.description,
}}
key={CSS.heading}
>
{heading}
</span>
</Heading>
) : null;

const descriptionNode = description ? (
<span aria-label={this.description} class={CSS.description} key={CSS.description}>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{description}
</span>
) : null;

return headingNode || descriptionNode ? (
<div class={CSS.container}>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{headingNode}
{descriptionNode}
</div>
) : null;
}

render(): VNode {
const { heading, description, thumbnail } = this;
const { thumbnail } = this;
return (
<Host>
<a class={CSS.anchor} href={this.href} rel={this.rel} target={this.target}>
{thumbnail && <img alt={this.label || ""} class={CSS.image} src={thumbnail} />}
{this.icon && this.renderIcon()}
{(heading || description) && (
<div class={CSS.container}>
{heading && (
<span
aria-label={this.heading}
class={{
[CSS.heading]: true,
[CSS.standalone]: !this.description,
}}
key={CSS.heading}
>
{heading}
</span>
)}
{description && (
<span aria-label={this.description} class={CSS.description} key={CSS.description}>
{description}
</span>
)}
</div>
)}
{this.renderHeaderContent()}
</a>
</Host>
);
Expand Down
6 changes: 6 additions & 0 deletions packages/calcite-components/src/demos/navigation.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -331,6 +331,12 @@ <h3>
<calcite-navigation-logo heading="Walt's Chips" slot="logo"> </calcite-navigation-logo>
</calcite-navigation>
</calcite-shell>
<calcite-shell>
<calcite-navigation slot="header" style="--calcite-color-brand: #bf390f">
<calcite-navigation-logo heading="Walt's Chips with Level" heading-level="3" slot="logo">
</calcite-navigation-logo>
</calcite-navigation>
</calcite-shell>
<calcite-shell>
<calcite-navigation slot="header" style="--calcite-color-brand: #bf390f">
<calcite-navigation-logo slot="logo" icon="link-chart"> </calcite-navigation-logo>
Expand Down
Loading