-
Notifications
You must be signed in to change notification settings - Fork 16
DEVPROD-1647 Add animation to details button when details menu items are changed. #424
Conversation
Passing run #4020 ↗︎
Details:
Review all test suite changes for PR #424 ↗︎ |
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.
Looks very cool 😎
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.
Oops I take it back! The animation still applies when a user manually changes the range which seems a bit aggressive.
Good call! |
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.
It seems like the animation also applies on initial load now
Screen.Recording.2023-11-07.at.10.23.42.AM.mov
I pushed up a fix that will prevent it from happening on production builds. Due to how react 18 renders things twice in dev mode the animation will only display on first render locally but shouldn't on a production build. (You can disable strict mode to test this) |
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.
Single line search in range selection isn't updating the details menu nor showing the animation. Those code changes can be made in a separate PR if you like.
I'm unable to figure out how to reset the search in range selection without opening the details menu. Is it possible?
} from "./Toggles"; | ||
|
||
interface DetailsMenuProps { | ||
["data-cy"]?: string; |
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.
["data-cy"]?: string; | |
"data-cy"?: string; |
expect(detailsButton).toHaveAttribute("data-variant", "primary"); | ||
jest.runAllTimers(); | ||
await waitFor(() => { | ||
expect(detailsButton).toHaveAttribute("data-variant", "default"); |
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.
nit: add assertions for all 3 states. flash off -> flash on -> flash off
src/components/DetailsMenu/index.tsx
Outdated
} | ||
const DetailsMenu: React.FC<DetailsMenuProps> = ({ disabled, ...rest }) => { | ||
const [lowerRange] = useQueryParam(QueryParams.LowerRange, 0); |
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.
const [lowerRange] = useQueryParam(QueryParams.LowerRange, 0); | |
const [lowerRange] = useQueryParam(QueryParams.LowerRange); |
We can open a ticket to update it if the flow doesn't make sense.
That is not possible. When we originally opened this pr the UX didn't make sense to reset the search range from the selection menu. It is why this pr exists so that users are made aware of the change in the details menu. |
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.
Good stuff!!
DEVPROD-1647
Description
Adds an animation to the details menu item when ever the search range is changed.
Screenshots
Screen.Recording.2023-10-31.at.3.52.37.PM.mov
Testing