-
-
Notifications
You must be signed in to change notification settings - Fork 33.7k
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
#4371 - Explicit transition durations #4857
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reviewed 👀
src/shared/util.js
Outdated
export function parseTime (text: String): number { | ||
const matched = text.match(/(\d+|\d+\.\d+|\.\d+)\s*(s|ms)/i) | ||
if (matched) { | ||
const [, durationText, unit] = matched |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo? [, durationText, unit]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first element in the array is the full match, so I skip it.
I think it's done. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work!
I think we should display a dev warning for NaN values on the duration to improve the DX
const beforeEnterHook = isAppear ? (beforeAppear || beforeEnter) : beforeEnter | ||
const enterHook = isAppear ? (typeof appear === 'function' ? appear : enter) : enter | ||
const afterEnterHook = isAppear ? (afterAppear || afterEnter) : afterEnter | ||
const enterCancelledHook = isAppear ? (appearCancelled || enterCancelled) : enterCancelled | ||
|
||
const explicitDuration = parseDuration(( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can call this enterDuration to be more explicit
@@ -175,6 +189,12 @@ export function leave (vnode: VNodeWithData, rm: Function) { | |||
// the length of original fn as _length | |||
(leave._length || leave.length) > 1 | |||
|
|||
const explicitDuration = parseDuration(( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same as above but with leaveDuration instead
@@ -179,3 +179,13 @@ function getTimeout (delays: Array<string>, durations: Array<string>): number { | |||
function toMs (s: string): number { | |||
return Number(s.slice(0, -1)) * 1000 | |||
} | |||
|
|||
export function parseDuration (value: Object): any { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can add the typing of number, NaN, or undefined to the return value, instead of any
src/shared/util.js
Outdated
* Returns the number of ms corresponding to the text expression. | ||
* @param text Time expression with a time unit ('ms' or 's') | ||
*/ | ||
export function parseTime (text: string): any { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the return type can be set to number
(and NaN I guess)
@@ -179,3 +179,24 @@ function getTimeout (delays: Array<string>, durations: Array<string>): number { | |||
function toMs (s: string): number { | |||
return Number(s.slice(0, -1)) * 1000 | |||
} | |||
|
|||
function parseDuration (value: any): number | typeof NaN | typeof undefined { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typeof NaN
is effectively number
in flow-type. https://flowtype.org/try/#0G4QwTgBCBcEC4E8AOBTA9gMwgORNiAvBAIwBMAzEA
And typeof undefined
is expressed with ?
by idiom in flow.
Thus the return type is equivalent to ?number
src/shared/util.js
Outdated
const matched: ?Array<string> = text.match(/(\d+|\d+\.\d+|\.\d+)\s*(s|ms)/i) | ||
if (matched) { | ||
/* eslint no-unused-vars:0 */ | ||
const [_, durationText: string, unit: string] = matched |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need explicitly annotate type here since matched must be an array of string. It should be straightforward to any qualified JS developer. Removing it will make code cleaner, I think.
This reverts commit db75b3801ed11182119c78ebae87f40a62803714.
Revert "Fix transition test" This reverts commit db75b3801ed11182119c78ebae87f40a62803714. Fix transition test
Thanks for the hard work! I decided to simplify the API a little bit so that it only accepts numbers. Personally I don't like APIs that do too much to cater to loose typing, and the amount of extra code/complexity needed to handle all the string formats are not really worth it. |
Add a new
duration
prop on<transition>
component (see #4371):