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.

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(calcite-link): Add support for download attribute. #3758 #3922

Merged
merged 13 commits into from
Jan 14, 2022
43 changes: 42 additions & 1 deletion src/components/calcite-link/calcite-link.e2e.ts
Original file line number Diff line number Diff line change
@@ -1,15 +1,56 @@
import { E2EElement, E2EPage, newE2EPage } from "@stencil/core/testing";
import { accessible, renders } from "../../tests/commonTests";
import { accessible, defaults, renders } from "../../tests/commonTests";

describe("calcite-link", () => {
it("renders", async () => renders("<calcite-link href='/'>link</calcite-link>", { display: "inline" }));

it("defaults", async () =>
defaults("calcite-link", [
{
propertyName: "download",
defaultValue: false
}
]));

it("is accessible", async () => {
await accessible("<calcite-link href='/'>link</calcite-link>");
await accessible("<calcite-link>link</calcite-link>");
await accessible("<calcite-link icon-start='plus' icon-end='plus' href='/'>Go</calcite-link>");
});

it("sets download attribute on internal anchor", async () => {
const page = await newE2EPage();
await page.setContent(`<calcite-link href="file.jpg">Continue</calcite-link>`);

const elementAsLink = await page.find("calcite-link >>> a");

expect(elementAsLink).not.toBeNull();
expect(await elementAsLink.getProperty("download")).toBe("");
expect(elementAsLink).not.toHaveAttribute("download");
Copy link
Member

Choose a reason for hiding this comment

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

For consistency, can these assertions use props vs attributes?

Copy link
Member Author

@driskull driskull Jan 13, 2022

Choose a reason for hiding this comment

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

Yes, but if I use the prop it will emit that console warning.

console.warn
    STENCIL: The state/prop "download" changed during rendering. This can potentially lead to infinite-loops and other bugs. 
    Element JSHandle@node 
    New value  
    Old value true 
    Location: http://localhost:3334/build/index-82db3f75.js:2914:41


const element = await page.find("calcite-link");

element.setProperty("download", true);
await page.waitForChanges();

expect(await elementAsLink.getProperty("download")).toBe("");
expect(elementAsLink).toHaveAttribute("download");
expect(elementAsLink.getAttribute("download")).toBe("");

const newFilename = "my-cool-file.jpg";
element.setProperty("download", newFilename);
await page.waitForChanges();

expect(await elementAsLink.getProperty("download")).toBe(newFilename);
expect(elementAsLink.getAttribute("download")).toBe(newFilename);

element.setProperty("download", false);
await page.waitForChanges();

expect(await elementAsLink.getProperty("download")).toBe("");
expect(elementAsLink).not.toHaveAttribute("download");
});

it("renders as a span with default props", async () => {
const page = await newE2EPage();
await page.setContent(`<calcite-link>Continue</calcite-link>`);
Expand Down
14 changes: 13 additions & 1 deletion src/components/calcite-link/calcite-link.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,12 @@ export class CalciteLink {
/** is the link disabled */
@Prop({ reflect: true }) disabled = false;

/** Prompts the user to save the linked URL instead of navigating to it. Can be used with or without a value:
* Without a value, the browser will suggest a filename/extension
* See: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/a#attr-download
*/
@Prop({ reflect: true }) download: string | boolean = false;

/** optionally pass a href - used to determine if the component should render as a link or an anchor */
@Prop({ reflect: true }) href?: string;

Expand Down Expand Up @@ -65,7 +71,8 @@ export class CalciteLink {
}

render(): VNode {
const dir = getElementDir(this.el);
const { download, el } = this;
const dir = getElementDir(el);

const iconStartEl = (
<calcite-icon
Expand Down Expand Up @@ -93,6 +100,11 @@ export class CalciteLink {
<Host role="presentation">
<Tag
class={{ [CSS_UTILITY.rtl]: dir === "rtl" }}
/*
When the 'download' property of type 'boolean | string' is set to true, the value is "".
This works around that issue for now.
*/
download={Tag === "a" && (download === "" || download) ? download : null}
Copy link
Member Author

@driskull driskull Jan 12, 2022

Choose a reason for hiding this comment

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

@jcfranco @eriklharper have you run into the following before...

  • If a property is property: boolean | string; and the property is set to true, the value is ""?
  • I had to work around this, I'm not sure if its as intended or stencil issue is needed?
  • I had expected the value of download to be true if set to true via a property but it was a "" empty string instead.
  • Basically, I have to treat the value of "" as true which seems counterintuitive and wrong 🤷

Copy link
Member Author

Choose a reason for hiding this comment

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

When setting via a prop we also get this lovely warning.

$0.download = true;
true
index-07dce265.js:2915 stencil The state/prop "download" changed during rendering. This can potentially lead to infinite-loops and other bugs. 
Element <calcite-link href=​"calcite-link.html" role=​"presentation" calcite-hydrated download>​…​</calcite-link>​ 
New value  
Old value true

I guess stencil just doesn't like mixed boolean string types?

Copy link
Member

Choose a reason for hiding this comment

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

Wow, interesting find. Can you add a comment there for posterity?

I'll create a repro case and send it over to the Stencil team.

href={Tag === "a" && this.href}
ref={this.storeTagRef}
rel={Tag === "a" && this.rel}
Expand Down
18 changes: 18 additions & 0 deletions src/demos/calcite-link.html
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,24 @@
</div>
</div>

<!-- download link -->
<div class="parent">
<div class="trigger-external-wrap-paragraph right-aligned-text">Download link: boolean</div>

<div class="trigger-external-wrap-paragraph">
<calcite-link download href="calcite-link.html">Download this page</calcite-link>
</div>
</div>

<!-- download link -->
<div class="parent">
<div class="trigger-external-wrap-paragraph right-aligned-text">Download link: value</div>

<div class="trigger-external-wrap-paragraph">
<calcite-link download="sample-page.html" href="calcite-link.html">Download this page</calcite-link>
</div>
</div>

<!-- wrapping -->
<div class="parent">
<div class="trigger-external-wrap-paragraph right-aligned-text">wrapping</div>
Expand Down