fix(alert): Sets autoCloseDuration to "medium" by default#7157
fix(alert): Sets autoCloseDuration to "medium" by default#7157
Conversation
github-actions
Bot
added
the
bug
benelan
left a comment
benelan
left a comment
There was a problem hiding this comment.
I didn't find a rule for that after a quick search, were you thinking we should add one to our custom calcite eslint plugin?
|
Yeah that's what I was thinking if its possible. |
jcfranco
left a comment
jcfranco
left a comment
There was a problem hiding this comment.
Aside from discussing the proposed convention separately, this LGTM!
|
|
||
| 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 |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
driskull
added
the
pr ready for visual snapshots
🤖 I have created a release *beep* *boop* --- <details><summary>@esri/calcite-components: 1.4.3</summary> ## [1.4.3](https://github.com/Esri/calcite-components/compare/@esri/calcite-components@1.4.2...@esri/calcite-components@1.4.3) (2023-06-26) ### Bug Fixes * **accordion-item:** support items working across shadowDOM ([#7035](#7035)) ([6378e35](6378e35)), closes [#6167](#6167) * **alert:** Sets autoCloseDuration to "medium" by default ([#7157](#7157)) ([1b9a8ed](1b9a8ed)) * **alert:** Update alert queue when an alert is removed from the DOM ([#7189](#7189)) ([edd59eb](edd59eb)) * **combobox, dropdown, input-date-picker, input-time-picker, popover, tooltip:** Prevent repositioning from affecting other floating components ([#7178](#7178)) ([1b02dae](1b02dae)) * Ensure mouse events are blocked for disabled components in Firefox ([#7107](#7107)) ([271d985](271d985)) * **input-date-picker:** Fix showing the placeholder when resetting the value ([#7156](#7156)) ([8d60ffd](8d60ffd)) * **input, input-number:** Allows numeric characters. ([#7213](#7213)) ([739f0af](739f0af)) * **input,input-number:** Allow typing decimal separator in firefox for arabic locale ([#7173](#7173)) ([595e6f2](595e6f2)) * **list:** No longer has incorrect border width ([a810943](a810943)) * **list:** Update selectedItems property on all item selection changes ([#7204](#7204)) ([da048f6](da048f6)) * **menu-item:** Ensure correct order of rendered icons ([#7098](#7098)) ([fd344e9](fd344e9)), closes [#7097](#7097) * **navigation:** Label is no longer a required property ([#7084](#7084)) ([ba2bd4d](ba2bd4d)) * **radio-button-group:** No longer focus first radio button on label click and adds `setFocus` method. ([#7050](#7050)) ([4267b8c](4267b8c)) * **radio-button, radio-button-group:** Prevent emitting events when selecting a checked radio button ([#7102](#7102)) ([77fcc81](77fcc81)) * **radio-button:** Focuses first focusable radio-button element in group. ([#7152](#7152)) ([dd7ec60](dd7ec60)) * **scrim:** Responsively set the scale of the loading spinner ([#7182](#7182)) ([72c5943](72c5943)) * **tooltip:** Improve component timing ([#7172](#7172)) ([106f5d2](106f5d2)) * **tree-item:** Ensure expanded tree-item is displayed when expanded and made visible ([#7216](#7216)) ([3c0fbf5](3c0fbf5)) <details><summary>@esri/calcite-components-react: 1.4.3</summary> ## [1.4.3](https://github.com/Esri/calcite-components/compare/@esri/calcite-components-react@1.4.2...@esri/calcite-components-react@1.4.3) (2023-06-26) ### Miscellaneous Chores * **@esri/calcite-components-react:** Synchronize undefined versions ### Dependencies * The following workspace dependencies were updated * dependencies * @esri/calcite-components bumped from 1.4.3-next.7 to 1.4.3 </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> Co-authored-by: Ben Elan <no-reply@benelan.dev>
3 participants
Related Issue: #6363
Summary
fix(alert): Sets autoCloseDuration to "medium" by default. #6363
@jcfranco
can we have linting to prevent having any conditional defaults? If not, we should maybe update conventions to say not to do thisLets discuss how we can properly document and test this pattern.