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

fix(alert): Sets autoCloseDuration to "medium" by default #7157

Merged
merged 3 commits into from
Jun 17, 2023
Merged

fix(alert): Sets autoCloseDuration to "medium" by default #7157

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d81a03a
fix(alert): Sets autoCloseDuration to "medium" by default. #6363
driskull Jun 8, 2023
cb0acb7
update conventions
driskull Jun 9, 2023
a74b0e0
revert convention
driskull Jun 15, 2023
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
16 changes: 16 additions & 0 deletions packages/calcite-components/conventions/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,22 @@ It is recommended to reflect properties that fit the following criteria:

Doing so will give developers more flexibility when querying the DOM. This is important in framework environments where we can't safely assume components will have their attributes set vs properties.

### Property defaults
Copy link
Member

Choose a reason for hiding this comment

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

In order to not block the fix, can we discuss this separately before making it a convention? There might be some other cases to consider (e.g., the color-picker ensures the format matches and set default value to hex or hexa depending on alpha being enabled or not).

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah lets discuss it.

As a user, its nice to know what the expected default value for a prop will be. Having these conditional default values does complicate things so we should discuss whether it matters or not.

We may be able to make rules for how these conditionals should be documented since there are other components like this. (such as datepicker value where it depends on if the component is a range or not)

We could at least update the doc for colorPicker to document the default since it only lists the hex right now. We may also want to enhance our defaults test to be able to handle these conditionals.

Copy link
Member Author

Choose a reason for hiding this comment

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

@jcfranco with the colorPicker, if alphaChannel is set to true after initialization, the value doesn't get updated right? Would a user expect the value to be updated? We should discuss that part as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe we can make exceptions for props that are mutable? In most cases value props are mutable and can be modified by the user and the component.


Properties should have static default values. There should be no conditional logic when defining the default value of a property.

#### Incorrect example

```ts
@Prop({ reflect: true }) autoCloseDuration: AlertDuration = this.autoClose ? "medium" : null;
```

#### Correct example

```ts
@Prop({ reflect: true }) autoCloseDuration: AlertDuration = "medium";
```

### `ref` usage

Due to a [bug in Stencil](https://github.com/ionic-team/stencil/issues/4074), `ref` should be set as the last property in JSX to ensure the node's attributes/properties are up to date.
Expand Down
11 changes: 10 additions & 1 deletion packages/calcite-components/src/components/alert/alert.e2e.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,18 @@
import { E2EElement, E2EPage, newE2EPage } from "@stencil/core/testing";
import { html } from "../../../support/formatting";
import { accessible, hidden, HYDRATED_ATTR, renders, t9n } from "../../tests/commonTests";
import { accessible, defaults, hidden, HYDRATED_ATTR, renders, t9n } from "../../tests/commonTests";
import { getElementXY } from "../../tests/utils";
import { CSS, DURATIONS } from "./resources";

describe("defaults", () => {
defaults("calcite-alert", [
{
propertyName: "autoCloseDuration",
defaultValue: "medium"
}
]);
});

describe("calcite-alert", () => {
const alertContent = `
<div slot="title">Title Text</div>
Expand Down
2 changes: 1 addition & 1 deletion packages/calcite-components/src/components/alert/alert.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ export class Alert implements OpenCloseComponent, LoadableComponent, T9nComponen
@Prop({ reflect: true }) autoClose = false;

/** Specifies the duration before the component automatically closes (only use with `autoClose`). */
@Prop({ reflect: true }) autoCloseDuration: AlertDuration = this.autoClose ? "medium" : null;
@Prop({ reflect: true }) autoCloseDuration: AlertDuration = "medium";

/** Specifies the kind of the component (will apply to top border and icon). */
@Prop({ reflect: true }) kind: Extract<
Expand Down