-
Notifications
You must be signed in to change notification settings - Fork 758
Fixes Line Highlight Bug #7768
Fixes Line Highlight Bug #7768
Conversation
@wldcordeiro Passed both the checks. 👍 |
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.
Minor note about a CSS variable but this looks good. I wonder if adding a test would be possible to avoid a regression. 🤔
const durationString = style.getPropertyValue( | ||
"--highlight-line-duration" | ||
); | ||
const duration = |
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.
Idea, avoid the parseFloat(...)
mess by changing the CSS var to 1500ms
then just strip out the ms
.
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.
@wldcordeiro But the stripped of "1500" would require a parseInt(...)
right?
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.
@wldcordeiro If it's not a big problem, are we good to go ahead with merging 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.
I really like that idea @wldcordeiro! WIth that we can just do a string replacement of ms
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.
@darkwing @wldcordeiro On it 👍
@darkwing @wldcordeiro Have a look at the changes. I've replaced it with 1500ms and stripped off the last two characters. |
Fixes #7757
Summary of Change
Test Plan
There wasn't any major change in the UI so I am planning to modify the tests later if required.
Demo