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

Enhancement: Alert - update autoDismissDuration to accept CSS time values #2800

Closed
asangma opened this issue Aug 12, 2021 · 26 comments
Closed
Labels
enhancement Issues tied to a new feature or request. need more info Issues that are missing information and/or a clear, actionable description.

Comments

@asangma
Copy link
Contributor

asangma commented Aug 12, 2021

Description

Update Alert's autoDismissDuration property to also accept valid CSS time values: ms and s.

Acceptance Criteria

Valid CSS duration values are accepted.
Existing strings continue to be accepted.

Relevant Info

Sometimes "fast" is too slow.

Which Component

Alert

Example Use Case

Showing a success message in a blocked ui.

@asangma asangma added enhancement Issues tied to a new feature or request. 0 - new New issues that need assignment. needs triage Planning workflow - pending design/dev review. labels Aug 12, 2021
@asangma
Copy link
Contributor Author

asangma commented Aug 12, 2021

Alternatively, make "fast" faster.
Or add "turbo".

@julio8a
Copy link

julio8a commented Sep 24, 2021

Tweaking values would be the best approach here. @asangma and @macandcheese can you guys work on this to find the optimal timing updates.

@julio8a julio8a removed the needs triage Planning workflow - pending design/dev review. label Sep 24, 2021
@julio8a julio8a added this to the Sprint 9/27 – 10/8 milestone Sep 24, 2021
@macandcheese
Copy link
Contributor

Will be hard to find values that please everyone, can we consider having a single default that is overridable (and has a minimum speed set and documented to prevent folks putting an alert up for .005 ms or whatever)

@jcfranco
Copy link
Member

Let's see if we can fine-tune the existing values first. Granted, it may not please everyone, but at least we'll have a consistent way of defining durations across components.

@macandcheese
Copy link
Contributor

I'm happy to defer to whatever values @asangma wants here.

@anveshmekala
Copy link
Contributor

@asangma please update the expected values of fast| turbo for calcite-alert in the description

@macandcheese
Copy link
Contributor

It does seem a bit weird to pass either a css value or a string as a value. Could we just update the underlying variables used and only accept slow/medium/fast?

Otherwise just removing the duration as a prop, and accepting a css variable to override a single default seems like another option.

I’m comfortable just making fast… faster if needed.

@anveshmekala anveshmekala added 2 - in development Issues that are actively being worked on. and removed 0 - new New issues that need assignment. labels Dec 28, 2021
@jcfranco
Copy link
Member

It does seem a bit weird to pass either a css value or a string as a value. Could we just update the underlying variables used and only accept slow/medium/fast?

👍

@anveshmekala
Copy link
Contributor

anveshmekala commented Dec 28, 2021

Here is the prototype of how it may look like if we allow users to control the autoDimissDuration.

cc: @macandcheese , @asangma , @jcfranco

@macandcheese
Copy link
Contributor

Yeah, I guess I don’t see a reason to let people set their own value here if we are already providing three speeds.

There is an open issue to pause the timer when hovering or focusing over the alert which would solve for the “dismisses too fast while trying to interact” case- but generally even having three seems overkill, imo.

@asangma thoughts on just adjusting the existing three values? I don’t have a preference there and happy to defer, but allowing custom timing seems too much. If needed users can just not use the auto dismiss setting at all.

@anveshmekala
Copy link
Contributor

Agree that allowing custom value would be a breaking change and to let the user's pause the timer when hovering/focusing would solve the issue of dismissing too fast.

Thanks @macandcheese .

@macandcheese
Copy link
Contributor

Issue for reference: #3338

@anveshmekala
Copy link
Contributor

For prototype purpose , i had reduced the duration scales by half and able to notice the difference.

@jcfranco
Copy link
Member

@asangma Do you have suggestions for updating the existing slow, medium, fast values? If not, let's go ahead and close this. cc @benelan

@asangma
Copy link
Contributor Author

asangma commented Dec 31, 2021

I think like-a-dis.

slow: 600ms
medium: 300ms
fast: 150ms

@anveshmekala
Copy link
Contributor

anveshmekala commented Dec 31, 2021

@asangma the alert will be dismissed in 0.15 seconds when set to fast. Is that supposed to be 1500ms by chance?

@asangma
Copy link
Contributor Author

asangma commented Dec 31, 2021

Wow wow...I totally mixed this up with a different discussion. Sorry.

I don't wanna mess with the default timing. The suggestion was to accept time values for autoDismissDuration.
So existing values ("slow", "medium", "fast") all still do the same thing, but a user could go like

auto-dismiss-duration="1200ms"
or
auto-dismiss-duration="12s"

and Alert we be like "Sweet, homie. I'll hang out for 12 seconds."

@anveshmekala
Copy link
Contributor

Referring to the comments above, i think the idea is to avoid the option of user's parsing time values as prop( ) or a css variable and fine tune the existing values.

cc @macandcheese , @jcfranco

@jcfranco
Copy link
Member

Correct. To reiterate, we'll skip setting actual values and keep that internal for now.

@asangma
Copy link
Contributor Author

asangma commented Dec 31, 2021

In that case, we might consider adding an "extra-fast" to AlertDuration. I know...I joked about "turbo", but it will be useful for certain use cases.

I think the existing values are good as-is.

If we add an "extra-fast" value, I'd suggest it follow the same 4000ms increment and be 2000ms.

@macandcheese
Copy link
Contributor

If we add an "extra-fast" value, I'd suggest it follow the same 4000ms increment and be 2000ms.

This seems almost too fast to be able to read anything other than a quick success / failure message:

Screen.Recording.2022-01-03.at.11.25.58.AM.mov

Is there an example of where this would be used - maybe some kind of more inline messaging or visual affordance could work?

Again I agree that the current fast isn't so .. fast - I think we can just shift all of the values to be a bit quicker.

@jcfranco
Copy link
Member

jcfranco commented Jan 6, 2022

Again I agree that the current fast isn't so .. fast - I think we can just shift all of the values to be a bit quicker.

👍🏼

@anveshmekala
Copy link
Contributor

@benelan can we close this issue considering we iterate the existing values instead of having a prop for users to pass the time value in ms| s ?

@jcfranco
Copy link
Member

@macandcheese @asangma Can you confirm if we need to tweak the values and if so, which values to use? Otherwise, let's close.

@macandcheese
Copy link
Contributor

Defer to @asangma here as the initial issue was his.

@anveshmekala anveshmekala added 0 - new New issues that need assignment. 2 - in development Issues that are actively being worked on. need more info Issues that are missing information and/or a clear, actionable description. and removed 2 - in development Issues that are actively being worked on. 0 - new New issues that need assignment. labels Jan 31, 2022
@asangma
Copy link
Contributor Author

asangma commented Feb 8, 2022

I think we can close.
Iterating the values seems like a better broad solution.

@asangma asangma closed this as completed Feb 8, 2022
@anveshmekala anveshmekala removed their assignment May 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Issues tied to a new feature or request. need more info Issues that are missing information and/or a clear, actionable description.
Projects
None yet
Development

No branches or pull requests

6 participants